1536154 - Add balanced tracking of external memory

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

As a first step towards precise tracking of all malloc memory (bug 1395509), we can track externally allocated memory precisely.

Comment 1

[Jon Coppeard (:jonco)]

Really this should talk about balanced tracking rather than precise tracking. We don't care if it's exactly correct but we do want to track both allocation and deallocation and we want them to match.

Comment 2

[Jon Coppeard (:jonco)]

The idea here is to replace all calls to JS_updateMallocCounter with a new API JS::AddAssociatedMemory and add calls to JS::RemoveAssociatedMemory such that these calls balance out and we can have a reasonable idea of how much external malloc memory is associated with GC things.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Add memory tracker to Zone r=sfink

Track external memory associated with JSObjects in a hash table keyed on GC thing and memory use. This ensures that memory associated with an object is correctly removed when the object is finalized. The different uses help to track down problems.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Eagerly run finalizer for any dead reflector JSObject when creating a new reflector for a DOM native r=bzbarsky

Currently incremental GC can run the finalizer for a dead reflector for a native after a new reflector for that native has been created and attached. This leads to the confusting situation where there are two reflectors that contain pointers to the same native (which has a pointer to the new one).

This is a problem for memory tracking because the finalizer needs to see the state of the native before the new reflector is attached.

The patch runs the finalizer for any dead reflector when creating a new one. This ensures that finalizer sees the correct state. The native object pointer is cleared when this happens so when the GC later runs the finalizer again it is a no-op. This situation occurs pretty rarely so I don't think there is much overhead to running the finalizer more than once. This also allows us to tighted up the assertions in the finalizer.

Depends on D28686

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Update JS_updateMallocCounter callers to use the new API r=bzbarsky

This updates existing callers to use the new JS::AddAssociatedMemory API and adds calls to RemoveAssociatedMemory in finalizers.

The associated memory doesn't need to be exact, so some simplifiations are made, e.g. in CanvasRenderingContext2D where we don't wait for memory to be allocated but update the number of bytes when the dimensions change, and for stream blobs where the value returned by SizeOfIncludingThis changes over the lifetime of the object.

Depends on D28690

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Update JS_updateMallocCounter callers in xpconnect to use the new APIs r=mccr8

Depends on D28692

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Count externally allocated malloc memory as part of total zone memory for scheduling purposes r=sfink

This will start triggering GCs based on this external memory count, without removing the existing malloc bytes trigger.

This should mean we collect earlier in some cases (hopefully those cases where we are not triggering frequently enough at the moment). Note that we clear the malloc bytes count on GC, so this doesn't mean we will trigger twice the number of collections.

The patch adds totalBytes() method to Zone and uses this for scheduling decisions. I removed the lock from GCRuntime::maybeAllocTriggerZoneGC because it doesn't seem to serve any purpose.

Depends on D28693

Comment 8

[Jon Coppeard (:jonco)]

I'm getting a GC hazard in generating bindings code calling BlobImpl::GetAllocationSize:

Function '_ZN7mozilla3dom12Blob_BindingL9_finalizeEPN2js6FreeOpEP8JSObject$BlobBinding.cpp:void mozilla::dom::Blob_Binding::_finalize(js::FreeOp*, JSObject*)' has unrooted 'obj' of type 'JSObject*' live across GC call '_ZN7mozilla3dom26BindingJSObjectMallocBytesEPNS0_4BlobE$uint64 mozilla::dom::BindingJSObjectMallocBytes(mozilla::dom::Blob*)' at obj-analyzed/dom/bindings/BlobBinding.cpp:785
BlobBinding.cpp:781: Call(1,2, self := UnwrapPossiblyNotInitializedDOMObject(obj*))
BlobBinding.cpp:782: Assume(2,3, !null(self*), true)
BlobBinding.cpp:783: Call(3,4, __temp_1 := UndefinedValue())
BlobBinding.cpp:783: Call(4,5, SetReservedSlot(obj*,0,__temp_1))
BlobBinding.cpp:784: Assume(5,7, !null(self*), false)
BlobBinding.cpp:784: Assign(7,8, __temp_2 := 0)
BlobBinding.cpp:784: Call(8,9, ClearWrapper(self*,__temp_2*,obj*))
BlobBinding.cpp:785: Call(9,10, mallocBytes := BindingJSObjectMallocBytes(self*)) [[GC call]]
BlobBinding.cpp:785: Assume(10,11, (mallocBytes* != 0), true)
BlobBinding.cpp:786: Call(11,12, RemoveAssociatedMemory(obj*,mallocBytes*,1))
GC Function: _ZN7mozilla3dom26BindingJSObjectMallocBytesEPNS0_4BlobE$uint64 mozilla::dom::BindingJSObjectMallocBytes(mozilla::dom::Blob*)
uint64 mozilla::dom::Blob::GetAllocationSize() const
mozilla::dom::BlobImpl.GetAllocationSize
unresolved mozilla::dom::BlobImpl.GetAllocationSize

This is a virtual method, but I'm pretty sure the analysis handles these now. Steve, any idea why this is saying it's unresolved?

Comment 9

[Jon Coppeard (:jonco)]

baku, I wanted to ask about the memory reporting of StreamBlobImpl. This bug is trying to give the JS engine a better idea
of how much memory is owned by DOM objects tied to JS reflector objects for scheduling purposes.

The problem I'm having is that the size returned by nsIStringInputStream::SizeOfIncludingThis can change over the lifetime
of the stream withouth the owning StreamBlobImpl knowing about it (e.g. when someone calls Close() on it).

I think this size is mostly a buffer used for the stream. Do you know if there's a way to get a less precise but more stable size for a nsIStringInputStream? Maybe there's a buffer size constant somewhere.

Alternatively, how widely are these used and how much memory do they tie up? Maybe it would be OK to ignore them for this purpose.

Comment 10

[Andrea Marchesini [:baku]]

StreamBlobImpl handles different kinds of streams (IPCBlobInputStream, nsIMultiplexInputStream, nsIFileInputStream and so on). We do memory reporting just for nsIStringInputStream (wondering if we should do something better, for instance when we have nsIMultiplexInputStream containing sub nsIStringInputStreams).

StreamBlobImpl knows the size of the blob because, in theory, blobs are immutable. For nsIStringInputStream, the length is always equal to the blob's size. But I need a better understanding of the problem:

StreamBlobImpl (as any other BlobImpl implementation) doesn't expose its inputStream. When a component calls: BlobImpl::CreateInputStream() the blob clones its own inputStream and it exposes the copy. This means that, nobody can call ::Close() on a StreamBlobImpl's inputStream. Because of this, the nsIStringInputStream's length should not change.

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Andrea Marchesini [:baku] from comment #10)

Thanks for the info.

the nsIStringInputStream's length should not change.

This does happen in practice though. Maybe it's not Close() that is causing this - that was just one possibility I saw when looking at the code.

For example, running the browser/base/content/test/pageActions/browser_page_action_menu.js mochitest we get a StreamBlobImpl whose size is reported as 128 when its reflector is created but as 4224 when that reflector is finalized.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: add-string-stream-accounting

To demonstrate the above failure requires the patches in this bug plus this one.

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #8)

I'm getting a GC hazard in generating bindings code calling BlobImpl::GetAllocationSize:

uint64 mozilla::dom::Blob::GetAllocationSize() const
mozilla::dom::BlobImpl.GetAllocationSize
unresolved mozilla::dom::BlobImpl.GetAllocationSize

This is a virtual method, but I'm pretty sure the analysis handles these now. Steve, any idea why this is saying it's unresolved?

As I eventually realized, because the analysis still doesn't properly handle these. It somewhat handles them, though it won't show its full reasoning in these GC stacks. The patch to properly handle them is still blocked on all the false positive hazards it uncovers (mostly related to QI). With that patch, I believe the GC stacks will be complete.

I'm going to take another look today to see if if I can come up with a usable annotation mechanism to suppress these hazards. I'd really like to get the new virtual method handling landed, but at the moment it's still a little too smart.

Comment 14

[Steve Fink [:sfink] [:s:]]

I hoped you could use https://queue.taskcluster.net/v1/task/WC20wMVlQNyACsn2fEbNlA/runs/0/artifacts/public/build/gcFunctions.txt.gz to see proper stacks for these, but it looks like that virtual method patch still needs some work. I'd be fine with a patch to annotations.js to just ignore GetAllocationSize in order to land this. Then once the virtual method patch lands, we can fix things properly. But it doesn't seem like there's a huge risk of actually GCing during that method, unless I'm missing something?

Comment 15

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #14)

But it doesn't seem like there's a huge risk of actually GCing during that method, unless I'm missing something?

No, the worst that happens is QIing an nsIInputStream to nsIStringInputStream.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: bug1536154-nsStringInputStream-memory-reporting

The problems above with a StreamBlob reporting a different size at different times came down to nsStringInputStream::SizeofIncludingThis which calls mData.SizeOfExcludingThisIfUnshared. This means that it can return a different size depending on the mData's refcount. I guess this is to prevent double counting in memory reporting.

I don't know where the extra ref count is coming from, but when it goes away it makes the reported size change which is a problem here.

Do you think it's OK to just make this use SizeOfExcludingThisEvenIfShared instead? I guess the alternative would be to give nsIStringInputStream another SizeOf like method to get the stable size but that doesn't seem great either.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 9061033 [details] [diff] [review]
bug1536154-nsStringInputStream-memory-reporting

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

I don't have a super-strong opinion here; I don't have a good sense of how often we really share string input streams in practice.  baku's comments make it sound like it could be pretty often, though?  So it feels like you're not only going to double-count in memory reporting, but you're also going to double-count in your malloc tracking?

Comment 18

[Jon Coppeard (:jonco)]

(In reply to Nathan Froyd [:froydnj] from comment #17)
OK thanks. In the light of all this I think it's probably easiest to give the string stream interface separate methods for getting the memory with/without shared memory.

I don't think double counting in the JS engine malloc tracking is going to be a problem as the memory use reported does not need not be exact.

Comment 19

[Jon Coppeard (:jonco)]

Attachment: Bug 1536154 - Make nsStringInputStream always report the full memory size of the backing string r=baku?

Give nsIStringStream separate SizeOfIncludingThisIfUnshared and SizeOfIncludingThisEvenIfShared methods. Use the former for memory reporting and the latter for JS engine memory accounting.

Depends on D28695

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e75c822fd3
Add memory tracker to Zone r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ca67e7684f
Eagerly run finalizer for any dead reflector JSObject when creating a new reflector for a DOM native r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/5159ad4a890b
Update JS_updateMallocCounter callers to use the new API r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf9348fa63c
Update JS_updateMallocCounter callers in xpconnect to use the new APIs r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1cf373a4e9f
Count externally allocated malloc memory as part of total zone memory for scheduling purposes r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1927f1c0f1e
Make nsStringInputStream always report the full memory size of the backing string r=baku

Comment 21

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/24e75c822fd3
https://hg.mozilla.org/mozilla-central/rev/b6ca67e7684f
https://hg.mozilla.org/mozilla-central/rev/5159ad4a890b
https://hg.mozilla.org/mozilla-central/rev/4cf9348fa63c
https://hg.mozilla.org/mozilla-central/rev/a1cf373a4e9f
https://hg.mozilla.org/mozilla-central/rev/e1927f1c0f1e