Closed
Bug 1342050
Opened 7 years ago
Closed 7 years ago
Shrink Promise instances from 8 to 4 slots
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
People
(Reporter: till, Assigned: till)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
10.83 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Promise instances currently have 8 slots[1]. 5 of those are used for debug information. After doing bug 1342047, we should move those into a lazily-allocated debug info object and shrink the Promise object itself to 4 slots. In the common case we'd never allocate the debug info object. Theoretically we could save an additional slot by moving the promise/debug info association to a weak map, but since we don't have OBJECT3 alloc kinds that wouldn't buy us anything. [1] http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/js/src/builtin/Promise.h#15-26
Updated•7 years ago
|
Whiteboard: [MemShrink]
Comment 1•7 years ago
|
||
Till, are you planning to work on this? How much of size savings are you expecting?
Flags: needinfo?(till)
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 2•7 years ago
|
||
I don't have near-term plans to work on it. I really don't know how much memory this'd save once bug 1342047 is done. However, it might actually be relevant to Quantum Flow: the reason I looked into this at all was that some profiling showed that allocation and GC was a meaningful part of the costs associated with Promise-heavy code.
Flags: needinfo?(till)
Whiteboard: [MemShrink:P2] → [MemShrink:P2] [qf]
Updated•7 years ago
|
Whiteboard: [MemShrink:P2] [qf] → [MemShrink:P2] [qf:p2]
Comment 3•7 years ago
|
||
Surely we know how big a slot is in memory? I guess I was curious how much memory this would save on a per-promise basis.
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #3) > Surely we know how big a slot is in memory? I guess I was curious how much > memory this would save on a per-promise basis. Oh, my apologies - I thought you're asking about expected overall savings. A slot is 8 bytes, so this'd save 32 bytes.
Comment 5•7 years ago
|
||
So per-promise size would go from 64 bytes to 32 bytes? Or is there some extra fixed overhead? I'm just wondering which jemalloc bin we are falling into...
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #5) > So per-promise size would go from 64 bytes to 32 bytes? Or is there some > extra fixed overhead? I'm just wondering which jemalloc bin we are falling > into... There's a 32 bytes header as overhead, so Promises would go from 96 bytes to 64 bytes. However, note that Promises are allocated in an arena in the gc heap, so jemalloc doesn't enter the picture and there can't be any slop.
Comment 7•7 years ago
|
||
Oh, I thought someone told me the c++ bits of promises would come out of jemalloc. Promises are completely js heap based? Sorry for my confusion.
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #7) > Oh, I thought someone told me the c++ bits of promises would come out of > jemalloc. Promises are completely js heap based? Sorry for my confusion. They're fully gc heap based, yes. The old DOMPromise implementation lived in the C++ heap, but that's gone now. The dom/Promise facade is stack-only, so that's not a problem either.
Assignee | ||
Comment 9•7 years ago
|
||
Looks like I wrote a patch for this after all. Promises going from 96 to 64 bytes doesn't have an immediate drastic impact on performance, but it does make a difference in that it puts less pressure on the GC. In various micro-benchmarks I see perf improvements of around 10% for the common case, and fewer spikes in runtime due to GC.
Comment 10•7 years ago
|
||
Comment on attachment 8899257 [details] [diff] [review] Shrink Promise instances from 8 to 4 slots Review of attachment 8899257 [details] [diff] [review]: ----------------------------------------------------------------- Nice :D ::: js/src/builtin/Promise.cpp @@ +182,5 @@ > + RootedObject stack(cx); > + if (!JS::CaptureCurrentStack(cx, &stack, JS::StackCapture(JS::AllFrames()))) > + return nullptr; > + debugInfo->setFixedSlot(Slot_AllocationSite, ObjectOrNullValue(stack)); > + debugInfo->setFixedSlot(Slot_AllocationTime, DoubleValue(MillisecondsSinceStartup())); Slot_ResolutionSite and Slot_ResolutionTime should be set to 0 and null here. @@ +214,5 @@ > + JSObject* resolutionSite() { return getFixedSlot(Slot_ResolutionSite).toObjectOrNull(); } > + > + static void setResolutionInfo(JSContext* cx, Handle<PromiseObject*> promise) { > + Rooted<PromiseDebugInfo*> debugInfo(cx, FromPromise(promise)); > + if (!debugInfo || !(cx->options().asyncStack() || cx->compartment()->isDebuggee())) it would be nice to add function for `cx->options().asyncStack() || cx->compartment()->isDebuggee()` condition (maybe as PromiseDebugInfo static method) and use it everywhere, since now it's in both PromiseObject and PromiseDebugInfo.
Attachment #8899257 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 11•7 years ago
|
||
Thank you for the quick review! Unfortunately a try run[1] surfaced a few issues I hadn't thought of, so here's a new version. I hope the interdiff works for this, but if not: the relevant changes are all in PromiseDebugInfo.
Attachment #8899257 -
Attachment is obsolete: true
Attachment #8899295 -
Flags: review?(arai.unmht)
Comment 12•7 years ago
|
||
Comment on attachment 8899295 [details] [diff] [review] Shrink Promise instances from 8 to 4 slots, v2 Review of attachment 8899295 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Promise.cpp @@ +205,5 @@ > + } > + > + /** > + * Returns the given PromiseObject's process-unique ID. > + * The ID is is lazily assigned when first queried, and then either stored s/is // ? @@ +231,5 @@ > + JSObject* allocationSite() { return getFixedSlot(Slot_AllocationSite).toObjectOrNull(); } > + JSObject* resolutionSite() { return getFixedSlot(Slot_ResolutionSite).toObjectOrNull(); } > + > + static void setResolutionInfo(JSContext* cx, Handle<PromiseObject*> promise) { > + Rooted<PromiseDebugInfo*> debugInfo(cx, FromPromise(promise)); this can be moved after the condition below.
Attachment #8899295 -
Flags: review?(arai.unmht) → review+
Comment 13•7 years ago
|
||
Pushed by tschneidereit@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/104866e1c85f Shrink Promise instances from 8 to 4 slots by moving debug information to an external object. r=arai
Comment 14•7 years ago
|
||
Pushed by tschneidereit@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2de35382cff Follow-up to fix a rooting hazard on a CLOSED TREE. r=me
Comment 15•7 years ago
|
||
Backed out for leaks in browser-chrome's browser_bug724239.js on Windows 8 x64: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed1d04cd2ecc16499a04b974c556ad089ed1825 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d080b90698bc6d062f46364a6ee666c4874eaef5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log browser_bug724239.js: https://treeherder.mozilla.org/logviewer.html#?job_id=125224640&repo=mozilla-inbound This one looks intermittent https://treeherder.mozilla.org/logviewer.html#?job_id=125224645&repo=mozilla-inbound > browser/components/migration/tests/browser/browser_undo_notification.js | leaked 2 window(s) until shutdown [url = about:newtab]
Flags: needinfo?(till)
Comment 16•7 years ago
|
||
Pushed by tschneidereit@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e324429501 Shrink Promise instances from 8 to 4 slots by moving debug information to an external object. r=arai
Assignee | ||
Comment 17•7 years ago
|
||
A try run shows that this isn't to blame for the leaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=386d468ed719c48a8b80381380600d720884058c
Flags: needinfo?(till)
Comment 18•7 years ago
|
||
Backed out bug 1342050 and bug 1386534 on suspicion of letting cgc's js/src/jit-test/tests/modules/missing-export-offthread.js fail (the Try run with the failure contained both patches): bug 1342050: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0a60ff0706d347c6156bc956713f25aecd4e60 bug 1386534: https://hg.mozilla.org/integration/mozilla-inbound/rev/443391e0208aff5594b5a9c5a0cc443d5918e3b5 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b9e3244295018feec4c99bd2e51b8374eb0ab44e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://queue.taskcluster.net/v1/task/H0wSUI-iQ6WCcMDc3aUqmg/runs/0/artifacts/public/logs/live_backing.log [task 2017-08-25T12:24:28.420647Z] TEST-PASS | js/src/jit-test/tests/v8-v5/check-regexp.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off") [task 2017-08-25T12:24:39.949030Z] Exit code: -9 [task 2017-08-25T12:24:39.949106Z] TIMEOUT - modules/missing-export-offthread.js [task 2017-08-25T12:24:39.949165Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/modules/missing-export-offthread.js | Timeout (code -9, args "--baseline-eager")
Flags: needinfo?(till)
Comment 19•7 years ago
|
||
Pushed by tschneidereit@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/592b32bafa97 Shrink Promise instances from 8 to 4 slots by moving debug information to an external object. r=arai
Assignee | ||
Comment 20•7 years ago
|
||
In my defense, I did look at a cgc failure in a try run, but happened to choose the one that was *another* intermittent. While this issue is very easily reproduced, it doesn't happen 100% of the time. Anyway, this bug is exonerated by a try run clearly fingering bug 1386534 as the culprit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c9f1beba56cf5b6bfcb0f55f54608a0d3d5f168
Flags: needinfo?(till)
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/592b32bafa97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Performance Impact: --- → P2
Whiteboard: [MemShrink:P2] [qf:p2] → [MemShrink:P2]
You need to log in
before you can comment on or make changes to this bug.
Description
•