Alon Zakai (:azakai) Reporter
	Description • 9 years ago

Comparing memory usage between Firefox and Chrome, Firefox uses a lot more memory when receiving large Blob XHRs. They appear to be kept in memory, while in Chrome, they seem to be kept on disk.

The attachment has a simple testcase that runs in a worker, and fetches 300MB of binary data in a Blob XHR. It then uses FileReaderSync to read from various locations in the file. In Chrome, memory usage hardly rises when loading the testcase; in Firefox, there is over 300MB of memory allocated for the Blob, showing up as "memory-file-data/large". This is also noticeable when viewing the OS processes.

Chrome's behavior where Blob XHRs are kept on disk would be helpful for applications with massive datasets, that can't even fit in memory. And FileReaderSync allows us to read from Blobs in a convenient manner in workers, which together would let us use massive datasets easily.

	Alon Zakai (:azakai) Reporter
	Comment 1 • 9 years ago

There are downsides to the Chrome approach as well. Firefox takes less than a second to load and read from those data files, while Chrome takes 4.5 seconds. Of which 3 seconds is the actual reads which for Firefox is just 0.023 (i.e. 23 ms). In other words, Chrome does a lot more disk IO here. Times are similar on two machines, on with an SSD and one without.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 9 years ago

We've talked about paging out large blobs to disk before, but it's never something we've felt necessary to implement.

	Alon Zakai (:azakai) Reporter
	Comment 3 • 9 years ago

I understand. It's hard for me to guess at what the best thing here is, but I am looking into ways to reduce memory usage for large applications (games, image editors, etc.), that have lots of data and currently are struggling to run on the web. Luke suggested Blob+FileReaderSync as one promising approach.

In some more testing now, I tried to load a Blob, then store it to IDB, then read it from there and use FileReaderSync. This still takes the full amount of memory at first, but once in IDB, it looks like we do actually keep the Blob on disk efficiently. However, the initial process of retrieving the file and storing it in IDB is very memory-intensive, perhaps there are even two copies in memory at some point?

In Chrome on the other hand, memory usage is almost flat during the whole process. It receives the XHR, stores it to IDB, loads it from there, and uses it, all without it ever being actually in memory. This does seem to support their approach here, I think.

Benchmarking the times also supports them: Unlike before, both browsers now do save the file on disk. Now the times are quite similar, suggesting that Chrome stores the XHR Blob and then repurposes it for IDB. So both browsers end up spending the time to write and then read, and Chrome avoids a double write.

I'll attach a testcase showing this.

	Alon Zakai (:azakai) Reporter
	Comment 4 • 9 years ago

Testcase of XHR => IDB => use Blob in FileReaderSync.

	Luke Wagner [:luke]
	Comment 5 • 9 years ago

khuey: I don't think we need anything fancy like paging out Blobs that were initially created from an in-memory source (like Blob(ArrayBuffer)), just for Blobs retrieved from the network.

	Alon Zakai (:azakai) Reporter
	Comment 6 • 9 years ago

Yes, I think Luke is right, after thinking about this some more I believe the issue here is binary XHRs where the user requested 'arraybuffer' vs 'blob' as the xhr.responseType.

It looks like right now in Firefox we handle memory the same in both cases, so the two are in effect almost the same, but in Chrome, when the user asked for a Blob, it is actually a normal on-disk Blob and memory is not used for it. That seems like very useful behavior.

	Masatoshi Kimura [:emk]
	Comment 7 • 9 years ago

We used to store XHR-blobs on the disk (bug 649133), but the ability was removed (bug 725993 and 758296), How does Chrome prevent XHR-blobs from running-out the disk space?

	Luke Wagner [:luke]
	Comment 8 • 9 years ago

Ah, good to know the backstory.  So, scanning bug 725993, it looks like there was every intention to use files to avoid the OOM issues we're seeing here and it was only removed because the HTTP cache was the wrong place for it (and it sounds like there was no quota checking in place at the time).  It also appears there was the intention to implement a new file-backed mechanism (shared by both WebSockets and XHR) but we just never got around to doing it b/c XHR+Blob were still new and so it was a lower priority.

I'm don't know what Chrome does, but I have to assume there is a quota mechanism to prevent disk exhaustion.

	Alon Zakai (:azakai) Reporter
	Comment 9 • 9 years ago

Perhaps we could use the same quota mechanism we already have for IndexedDB, precompiled asm.js binaries, etc.? (and if a blob xhr fails the quota, we'd store it in memory; and if it is on disk, we delete it when the page is closed)

	Marco Castelluccio [:marco]
	Comment 10 • 9 years ago

This would have been useful for PluotSorbet (j2me.js) as well, where we implemented a FileSystem on top of IndexedDB.

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 8 years ago

We have an interesting implementation in EncodedBufferCache (See /dom/media/EncodedBufferCache.*) where we go from a MemoryBlobImpl to a TemporaryBlobImpl when the buffer size is bigger than 1mb. Maybe we can use the same approach here.
Are we still interested in having this feature?

	Alon Zakai (:azakai) Reporter
	Comment 12 • 8 years ago

Yes, definitely still interested in this feature.

	Andrea Marchesini [:baku] Assignee
	Comment 14 • 8 years ago

Smaug, I don't know who else can review these patches. If you are too busy, just move to them to somebody else. Thanks!

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 8 years ago

Comment on attachment 8763118 [details] [diff] [review]
part 2 - Memory to Temporary BlobImpl

Review of attachment 8763118 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.h
@@ +1300,5 @@
>  
> +  uint32_t
> +  MaxMemoryForTemporaryFile() const
> +  {
> +    return mMaxMemoryForTemporaryFile;

This value is not initialized in the CTOR of WorkerPrivate. I did it. If you want I can upload a new patch with this change.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 8 years ago

The good thing of these patches is that it works also for normal blobs. This means that if the user does:

var a = ""; for (var i = 0; i < 2048; ++i) { a += "hello world!"; }
var blob = new Blob([a]);

this is written in a temporary file on disk.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 8 years ago

but only if the size of the blob is > 1024000 ? And at which point to we write to disk? And do we do it even in private browsing mode?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 8 years ago

How does the patch work in e10s, assuming the child process is sandboxed?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 8 years ago

Comment on attachment 8763311 [details] [diff] [review]
part 2 - Memory to Temporary BlobImpl

This is doing main thread I/O. We don't want that.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 8 years ago

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #19)
> How does the patch work in e10s, assuming the child process is sandboxed?

Right. We have to support private Browsing. For e10s, if the I/O fails, we continue in memory. This should be enough.

About the I/O on main-thread, yes... I can try to move everything to an I/O thread. The code will be a bit more complex because I must play with mutex and refcounting, but it makes sense to try.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 22 • 8 years ago

(In reply to Andrea Marchesini (:baku) from comment #21)
> Right. We have to support private Browsing. For e10s, if the I/O fails, we
> continue in memory. This should be enough.
Is it? Don't we plan to have sandboxed content processes everywhere, in which case the code wouldn't ever succeed to store the data in a file, except when chrome js uses Blobs.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 8 years ago

Comment on attachment 8763117 [details] [diff] [review]
part 1 - BlobSet - indentation, moving code, etc

>+
>+  if (!bufferLen.isValid())
>+    return false;
>+
>+  void* data = realloc(mData, bufferLen.value());
>+  if (!data)
>+    return false;
>+

since you're anyhow doing random other changes too, want to put {} to these ifs

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 24 • 8 years ago

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #19)
> How does the patch work in e10s, assuming the child process is sandboxed?

Good news: NS_OpenAnonymousTemporaryFile (which the patch uses) already knows how to message the parent to do the filesystem operations and send back a file descriptor/handle.

Bad news: it does sync IPC and main thread I/O, even if called off-main-thread in the content process; see bug 1149313.

	Andrea Marchesini [:baku] Assignee
	Comment 25 • 8 years ago

about the bad news: do you think this should be handled in this patch, or I can propose a patch as I wrote and leave the main-thread I/O issue for bug 1149313 ?

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 26 • 8 years ago

My inclination would be to just use NS_OpenAnonymousTemporaryFile as-is and defer fixing it to bug 1149313 — it's possible that the longer-term fix might be to give content processes access to a writable temporary directory because they need it anyway on desktop.  But people with more experience on the DOM side might be more concerned about the blocking/syncness here.  (To clarify, that's only about creating/opening the file; the writes themselves can be done off-main-thread in the child and won't block anything else.)

	Drake
	Comment 27 • 8 years ago

Lufi [1], a file upload/download service, is limited by this behavior. It is downloading a file from encrypted chunks on a server (through websocket), decrypting each one and storing the result in a memory object. Then it creates a blob from this memory object for the user to download. The problem is that the memory used is twice the size of the downloaded object during the blob creation, and it hits the available computer memory quite often on big files (don't even mentioning 32bit systems). I think solving this ticket would be of much help for this service. Or is there another way to do it?

Lufi is used by framasoft for their framadrop [2] service, as part of their "De-google-ify Internet" [3] initiative. This service is a libre alternative to wetransfer or mega.

[1] https://framagit.org/luc/lufi/
[2] https://framadrop.org/
[3] https://degooglisons-internet.org/

	Andrea Marchesini [:baku] Assignee
	Comment 28 • 8 years ago

At the moment this is just a subset of what BlobSet does.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 31 • 8 years ago

Comment on attachment 8792937 [details] [diff] [review]
part 1 - MutableBlobStorage for XHR

Though, do we want to keep using BlobSet with moz_blob.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 34 • 8 years ago

Comment on attachment 8793830 [details] [diff] [review]
part 3 - BlobSet and MutableBlobStorage for XHR

odd, I thought I had reviewed this before.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 8 years ago

Comment on attachment 8793831 [details] [diff] [review]
part 4 - temp files


>+    // We want to wait until all the WriteRunnable are completed. The way we do
>+    // this is to go to the I/O thread and then we come back: the runnables are
>+    // executed in order and this LastRunnable will be... the last one.
>+    nsresult rv = DispatchToIOThread(new LastRunnable(this, aParent,
>+                                                      aContentType, aCallback));
This doesn't work during shutdown, and that means that WriteRunnable might be the last thing to keep 
MutableBlobStorage alive and it might get deleted in non-main-thread. I guess that is fine, since at that moment MutableBlobStorage doesn't keep
non-main-thread alive. right?

Looks good, but, please add couple of more tests in a separate patch. Something running on workers, and both main thread and worker test should also test that
what happens when XHR is abort()'ed at random time before the resource has been loaded.

So, I'm expecting to see at least one more patch in this bug, but r+ to this one.

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 8 years ago

> non-main-thread alive. right?

Yes. It's fine. What it will happen is:

1. the DTOR or MutableBlobStorage create a CloseFileRunnable runnable trying to dispatch it to the I/O thread.
2. also this runnable will not be dispatched, but the DTOR of CloseFileRunnable will close the fd (on the main-thread).

	Andrea Marchesini [:baku] Assignee
	Comment 37 • 8 years ago

I'm not sure we want to land this test because if you run it with --repeat X, after a while the server stops to respond. I guess it's because of the abort and the request is truncated and not correctly handled on the server side.

	Andrea Marchesini [:baku] Assignee
	Comment 39 • 8 years ago

This patch works with --repeat.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 40 • 8 years ago

Comment on attachment 8795680 [details] [diff] [review]
part 5 - tests

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  c8705bdf03f64f22341e991a336f03ff78339c64
>
>diff --git a/dom/xhr/tests/common_temporaryFileBlob.js b/dom/xhr/tests/common_temporaryFileBlob.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/xhr/tests/common_temporaryFileBlob.js
>@@ -0,0 +1,101 @@
>+var data = "";
>+for (var i = 0; i < 256; ++i) data += "1234567890ABCDEF";

var data = new Array(256).join("1234567890ABCDEF") might be a bit faster, but probably
doesn't matter since the string isn't super long anyhow.

	Pulsebot
	Comment 41 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/218abd044ee6
Memory Blob to Temporary File - part 1 - MutableBlobStorage for XHR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/36f028447d58
Memory Blob to Temporary File - part 2 - BlobSet just for MultipartBlobImpl, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd96b988887
Memory Blob to Temporary File - part 3 - BlobSet and MutableBlobStorage for XHR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/13637a65df3b
Memory Blob to Temporary File - part 4 - Temporary File, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d109b8a96f0
Memory Blob to Temporary File - part 5 - tests, r=smaug

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=36625139&repo=mozilla-inbound

	Pulsebot
	Comment 43 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a02c9ac8e8
Backed out changeset 2d109b8a96f0 
https://hg.mozilla.org/integration/mozilla-inbound/rev/7539308f9f91
Backed out changeset 13637a65df3b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/996b454a69f0
Backed out changeset abd96b988887 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6fcaab4f16
Backed out changeset 36f028447d58 
https://hg.mozilla.org/integration/mozilla-inbound/rev/6877dcb886c0
Backed out changeset 218abd044ee6 for bustage

	Pulsebot
	Comment 44 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249aeef1b718
Memory Blob to Temporary File - part 1 - MutableBlobStorage for XHR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba90bde149a
Memory Blob to Temporary File - part 2 - BlobSet just for MultipartBlobImpl, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1af9d87ecd
Memory Blob to Temporary File - part 3 - BlobSet and MutableBlobStorage for XHR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb086e0404f
Memory Blob to Temporary File - part 4 - Temporary File, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c634201ba01d
Memory Blob to Temporary File - part 5 - tests, r=smaug

	Carsten Book [:Tomcat]
	Comment 45 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/249aeef1b718
https://hg.mozilla.org/mozilla-central/rev/4ba90bde149a
https://hg.mozilla.org/mozilla-central/rev/5b1af9d87ecd
https://hg.mozilla.org/mozilla-central/rev/aeb086e0404f
https://hg.mozilla.org/mozilla-central/rev/c634201ba01d