Make the number of reserved slots dynamic for proxy classes

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Bug 1237504 hardcoded this to 2 but it should be trivial to change that now.

It will let us shrink the size of DOM proxies because they need only 1 reserved slot instead of 2, so combined with the private slot it means we can go from an OBJECT4 allocation to OBJECT2.

I also want to use a constexpr template to static_assert the number of slots <= MAX_FIXED_SLOTS.
(Assignee)

Comment 1

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> I also want to use a constexpr template to static_assert the number of slots
> <= MAX_FIXED_SLOTS.

(Slots including the private slot, of course.)
(Assignee)

Comment 2

2 years ago
Posted patch PatchSplinter Review
This patch defines the number of slots explicitly for each proxy Class.

We can use a constexpr template to assert a few things about our Class flags. I can compile (and start) the browser with this, fingers crossed GCC and MSVC accept this too (I'll do a Try push of course).

I verified the static_assert works correctly: if I give a Proxy class 15 slots it compiles and with 16 slots compilation fails.

Unfortunately I had to move this code from jsfriendapi.h to js/public/Proxy.h, because we need ProxyValueArray defined there and Proxy.h includes jsfriendapi.h. Sorry about that.
Attachment #8862889 - Flags: review?(bzbarsky)
Comment on attachment 8862889 [details] [diff] [review]
Patch

>+++ b/dom/base/WindowNamedPropertiesHandler.cpp
>+                  JSCLASS_HAS_RESERVED_SLOTS(1)),

Strictly speaking, this class doesn't need any reserved slots at all.  It certainly never uses them.

Maybe document that we just have this to keep the SpiderMonkey asserts happy?

r=me; thank you for doing this!
Attachment #8862889 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

2 years ago
Thanks for the quick review! I'll add the comment and push this later today or tomorrow.

(In reply to Jan de Mooij [:jandem] from comment #2)
> fingers crossed GCC and MSVC accept this too

They do \o/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7844b2c91af47a2051f6d11943f59ad00bdd7654&group_state=expanded

Comment 5

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5d4015f0c1
Define number of reserved slots explicitly for each proxy js::Class. r=bz
https://hg.mozilla.org/mozilla-central/rev/1d5d4015f0c1
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.