Allocate ProxyValueArray inline

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Posted patch Patch (obsolete) — 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

2 years ago
Posted patch Patch (obsolete) — Splinter Review
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

2 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

2 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

2 years ago
Posted patch Patch (obsolete) — Splinter Review
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

2 years ago
Posted patch PatchSplinter Review
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)
(Assignee)

Updated

2 years ago
Blocks: 1351769
Whiteboard: [qf]

Updated

2 years ago
Whiteboard: [qf] → [qf:p3]
Attachment #8860686 - Flags: review?(bhackett1024) → review+

Comment 6

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d8027e7b9d7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.