922429 - GSN cache memory is leaking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

Seen after a day's usage:

Unreported: ~1,972 blocks in stack trace record 5 of 8,620
 ~8,071,396 bytes (~8,071,396 requested / ~0 slop)
 0.91% of the heap (5.30% cumulative);  3.57% of unreported (20.78% cumulative)
 Allocated at
   replace_calloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1246) 0x7fdcb0f20edb
   js::detail::HashTable<js::HashMapEntry<unsigned char*, unsigned char*>, js::HashMap<unsigned char*, unsigned char*, js::PointerHasher<unsigned char*, 0ul>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::init(unsigned int) (/opt/build/froydnj/build-mc/js/src/./../../dist/include/js/HashTable.h:992) 0x7fdcae2caaec
   js::jit::IonBuilder::snoopControlFlow(JSOp) (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1220) 0x7fdcae360b2b
   js::jit::IonBuilder::traverseBytecode() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1124) 0x7fdcae361fd0
   js::jit::IonBuilder::build() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:597) 0x7fdcae365c85
   IonCompile (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1620) 0x7fdcae342bd7
   js::jit::CompileFunctionForBaseline(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, bool) (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1960) 0x7fdcae344038
   EnsureCanEnterIon (/home/froydnj/src/mozilla-central-official/js/src/jit/BaselineIC.cpp:741) 0x7fdcae3021e0
   ??? 0x7fdc9f65186a

Unreported: ~604 blocks in stack trace record 9 of 8,620
 ~3,558,407 bytes (~3,015,687 requested / ~542,720 slop)
 0.40% of the heap (7.68% cumulative);  1.57% of unreported (30.14% cumulative)
 Allocated at
   replace_calloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1246) 0x7fdcb0f20edb
   js::detail::HashTable<js::HashMapEntry<unsigned char*, unsigned char*>, js::HashMap<unsigned char*, unsigned char*, js::PointerHasher<unsigned char*, 0ul>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::init(unsigned int) (/opt/build/froydnj/build-mc/js/src/./../../dist/include/js/HashTable.h:992) 0x7fdcae2caaec
   js::jit::IonBuilder::jsop_ifeq(JSOp) (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:3300) 0x7fdcae35da57
   js::jit::IonBuilder::inspectOpcode(JSOp) (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1285) 0x7fdcae361d70
   js::jit::IonBuilder::traverseBytecode() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1157) 0x7fdcae362134
   js::jit::IonBuilder::build() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:597) 0x7fdcae365c85
   IonCompile (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1620) 0x7fdcae342bd7
   js::jit::CompileFunctionForBaseline(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, bool) (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1960) 0x7fdcae344038
   EnsureCanEnterIon (/home/froydnj/src/mozilla-central-official/js/src/jit/BaselineIC.cpp:741) 0x7fdcae3021e0
   ??? 0x7fdc9f65186a

Unreported: 107 blocks in stack trace record 27 of 8,620
 876,544 bytes (657,408 requested / 219,136 slop)
 0.10% of the heap (11.99% cumulative);  0.39% of unreported (47.04% cumulative)
 Allocated at
   replace_calloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1246) 0x7fdcb0f20edb
   js::detail::HashTable<js::HashMapEntry<unsigned char*, unsigned char*>, js::HashMap<unsigned char*, unsigned char*, js::PointerHasher<unsigned char*, 0ul>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::init(unsigned int) (/opt/build/froydnj/build-mc/js/src/./../../dist/include/js/HashTable.h:992) 0x7fdcae2caaec
   js::jit::IonBuilder::jsop_ifeq(JSOp) (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:3300) 0x7fdcae35da57
   js::jit::IonBuilder::inspectOpcode(JSOp) (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1285) 0x7fdcae361d70
   js::jit::IonBuilder::traverseBytecode() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:1157) 0x7fdcae362134
   js::jit::IonBuilder::build() (/home/froydnj/src/mozilla-central-official/js/src/jit/IonBuilder.cpp:597) 0x7fdcae365c85
   IonCompile (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1620) 0x7fdcae342bd7
   js::jit::CanEnter(JSContext*, js::RunState&) (/home/froydnj/src/mozilla-central-official/js/src/jit/Ion.cpp:1932) 0x7fdcae344217
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2511) 0x7fdcae13b4e5
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:419) 0x7fdcae1402c6
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:388) 0x7fdcae140587
   js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:512) 0x7fdcae1425cd
   DoCallFallback (/home/froydnj/src/mozilla-central-official/js/src/jit/BaselineIC.cpp:7575) 0x7fdcae31e5db
   ??? 0x7fdc9f650270

Comment 1

[Nicholas Nethercote [inactive]]

I can do this one, though maybe not until after the summit ends.

Comment 2

[Nicolas B. Pierron [:nbp]]

This looks like the GSNCache which is the hash table used to cache the source notes during Ion compilation.  AFAIK, they are not supposed to survive after Ion compilations.

Could that be a side-effect of the memory reporter which trigger an Ion compilation before reporting the memory of the compilation that it just triggered?

CC: bhackett

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

I think the GSN cache memory is leaking.  This structure uses the malloc heap to allocate and is a member of IonBuilder, but since top level IonBuilders are not explicitly destroyed the memory is not released.  In general, IonBuilder data structures are supposed to use the IonAllocPolicy to allocate from the temp allocator associated with the build, but since GSNCache is used elsewhere in the VM I think it would be annoying to try to do that here.  This patch just releases the GSN cache memory after the top level builder finishes, since it won't be used anymore.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 812658 [details] [diff] [review]
patch

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

This seems fine but IonBuilder also has a ScopedJSDeletePtr<TypeRepresentationSetHash>, that will also be leakead I think. Can we delete IonBuilder in FinishOffThreadBuilder and use a ScopedJSDeletePtr<IonBuilder> in IonCompile?

Comment 5

[Brian Hackett [Laid off!]]

Yeah, that will leak, though that's a separate bug from this.  Even if the top level IonBuilder is explicitly destroyed there will still be plenty of objects whose destructors are never called, e.g. intermediate things allocated with IonAllocPolicy in IonBuilder and later compilation phases.  At one point I think I audited these looking for SystemAllocPolicy, but the GSN cache and type representation set things are newer changes.

Would it make more sense to just insert checks in the system malloc that it never gets called during Ion compilation, except during the (soon to be eliminated, per bug 785905) callouts to other parts of the VM?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 812658 [details] [diff] [review]
patch

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

(In reply to Brian Hackett (:bhackett) from comment #5)
> Would it make more sense to just insert checks in the system malloc that it
> never gets called during Ion compilation, except during the (soon to be
> eliminated, per bug 785905) callouts to other parts of the VM?

Yeah I think that's a good idea.

Comment 7

[Brian Hackett [Laid off!]]

This patch just fixes the GSN cache leak, as that fix should get uplifted.  I'll write up a patch for more comprehensive leak detection in the compiler.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d7cfcd7f2f

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 812658 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 917952
User impact if declined: small memory leak
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): none

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> Would it make more sense to just insert checks in the system malloc that it
> never gets called during Ion compilation, except during the (soon to be
> eliminated, per bug 785905) callouts to other parts of the VM?

The SystemAllocPolicy is *really* useful when we have MIR transformation phases which needs extra memory to complete the transformation.  In such case, we I would also agree that we could use a second LIFO Alloc which mirror the stack.  It would be sad to fill-up the space of the LifoAlloc used to keep things alive until the code gen because we have a few large temporary vectors in the middle.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a4d7cfcd7f2f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ed182212ef3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

So it probably would have helped if this had landed after bug 917952, eh? Backed out until that one gets approval.
https://hg.mozilla.org/releases/mozilla-aurora/rev/bbb0113e1216

Fail.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 812658 [details] [diff] [review]
patch

Bug 917952 didn't land on 26, so this doesn't need to either per bhackett.