Closed
Bug 1358753
Opened 8 years ago
Closed 8 years ago
Allocate ProxyValueArray inline
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 3 obsolete files)
17.52 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
For bug 1237504 we want to have reserved slots on proxies. When bz and I were talking about that yesterday, I realized we could get a very easy perf win right now by allocating ProxyValueArray inline in the object. This eliminates a malloc for each non-nursery allocated proxy/wrapper and improves cache locality.
This patch still keeps the ProxyValueArray* pointer. We could remove it now and get rid of the dereference, but since we have JIT code and friend APIs poking at these values it seemed better to do that separately.
Attachment #8860614 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 1•8 years ago
|
||
Oops, we also need to update the ProxyValueArray* pointers in JSObject::swap...
Attachment #8860614 -
Attachment is obsolete: true
Attachment #8860614 -
Flags: review?(bhackett1024)
Attachment #8860619 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8860619 [details] [diff] [review]
Patch
Still some orange on Try from JSObject::swap :(
Attachment #8860619 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•8 years ago
|
||
To support JSObject::swap, I wonder if we should keep the ProxyValueArray* pointer and make it so that it points either into the object itself (the common case) or to malloc'd data (when swapping with a smaller native object).
Then bug 1237504 could make the number of Values in ProxyValueArray dynamic (have it depend on the Class).
I think that would work but it's pretty unfortunate that we can't always store these slots inline...
Assignee | ||
Comment 4•8 years ago
|
||
This version still allocates ProxyValueArray inline, but JSObject::swap now uses malloc when the object sizes don't match. It seems to work locally: unlike the previous version, a debug build can load Gmail without crashing.
So we can't remove the ProxyValueArray* pointer, but we still eliminate a malloc call for 99% of proxies.
Unfortunately there's an infra issue so Try will have to wait.
Attachment #8860619 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Here we go. This version looks green on Try so far.
I added some logging and this should eliminate about 20,000 mallocs when starting Firefox. I also checked Dromaeo and there we're talking millions (many non-Nursery-allocated DOM proxies) so hopefully we'll see a perf win there.
Attachment #8860632 -
Attachment is obsolete: true
Attachment #8860686 -
Flags: review?(bhackett1024)
Updated•8 years ago
|
Whiteboard: [qf] → [qf:p3]
Updated•8 years ago
|
Attachment #8860686 -
Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8027e7b9d7
Allocate ProxyValueArray inline in the object instead of using malloc. r=bhackett
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•3 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in
before you can comment on or make changes to this bug.
Description
•