Closed Bug 922429 Opened 6 years ago Closed 6 years ago

GSN cache memory is leaking

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: froydnj, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink][qa-])

Attachments

(1 file)

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
I can do this one, though maybe not until after the summit ends.
Assignee: nobody → n.nethercote
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
Blocks: 917952
Attached patch patchSplinter Review
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.
Assignee: n.nethercote → bhackett1024
Attachment #812658 - Flags: review?(jdemooij)
Summary: various bits of ion compilation memory are not reported → GSN cache memory is leaking
Whiteboard: [MemShrink]
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?
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 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.
Attachment #812658 - Flags: review?(jdemooij) → review+
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 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
Attachment #812658 - Flags: approval-mozilla-aurora?
(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.
https://hg.mozilla.org/mozilla-central/rev/a4d7cfcd7f2f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #812658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Whiteboard: [MemShrink] → [MemShrink][don't uplift until bug 917952 is approved]
Whiteboard: [MemShrink][don't uplift until bug 917952 is approved] → [MemShrink]
Comment on attachment 812658 [details] [diff] [review]
patch

Bug 917952 didn't land on 26, so this doesn't need to either per bhackett.
Attachment #812658 - Flags: approval-mozilla-aurora+
Whiteboard: [MemShrink] → [MemShrink][qa-]
You need to log in before you can comment on or make changes to this bug.