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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Performance Impact medium
Tracking Status
firefox54 --- affected
firefox57 --- fixed

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)

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
Whiteboard: [MemShrink]
Till, are you planning to work on this? How much of size savings are you expecting?
Flags: needinfo?(till)
Whiteboard: [MemShrink] → [MemShrink:P2]
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]
Whiteboard: [MemShrink:P2] [qf] → [MemShrink:P2] [qf:p2]
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.
(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.
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...
(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.
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.
(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.
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.
Assignee: nobody → till
Status: NEW → ASSIGNED
Attachment #8899257 - Flags: review?(arai.unmht)
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+
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 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+
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
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
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
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)
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)
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
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)
https://hg.mozilla.org/mozilla-central/rev/592b32bafa97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.

Attachment

General

Created:
Updated:
Size: