Closed Bug 970498 Opened 6 years ago Closed 11 months ago

put JSClass on a diet

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file)

JSClass has a large reserved slot:

http://mxr.mozilla.org/mozilla-central/source/js/public/Class.h#528

That's 168/336 bytes for 32/64-bit, for those of you playing along at home.  For the vast majority of the ~1100 instances of JSClass in a build (and >99% of these come from bindings code), we don't use any of those fields.  So that's ~170K or so of wasted space.

Can we figure out a plan (a flag...something) to make those fields unnecessary?  Talking with efaust on IRC, he says this sounds "stupidly risky", and it looks like there are indeed some thorny issues to deal with, but for 170K+ of space savings, I am willing to do some stupid things.
Just make |reserved| private and route all accesses through (checked at debug-time) accessors... and then wonder if the JITs generate code that accesses it directly.
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Just make |reserved| private and route all accesses through (checked at
> debug-time) accessors... and then wonder if the JITs generate code that
> accesses it directly.

They sure do! See Class::offsetOfFlags(). There's at least one more coming down the pipeline. Any scheme that tries to remove |reserved| needs to be jit friendly. At least we can tell which fields are being used by the jits easily while editing that file.

Some accessors are also in the works already, so this route makes a lot of sense. I am planning to make call and construct into methods because I have to handle some stuff for proxies, and more seem easy to do.
(In reply to Eric Faust [:efaust] from comment #2)
> They sure do! See Class::offsetOfFlags().

But Class::flags isn't in the reserved area.

> There's at least one more coming down the pipeline.

What bug? Which fields?
Here's a first step: Let's not pad *two* classes; let's just pad one out, and
then static asserts will ensure that they're the same size.
Attachment #8380827 - Flags: review?(jorendorff)
Comment on attachment 8380827 [details] [diff] [review]
remove unnecessary padding in JSClass and js::Class

lulz
Attachment #8380827 - Flags: review?(jorendorff) → review+
Landed with the addition of removing the now-unused ClassSizeMeasurement.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bdecc9baac
Flags: in-testsuite-
Leaving open for further trimming of JSClass.

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> (In reply to Eric Faust [:efaust] from comment #2)
> > They sure do! See Class::offsetOfFlags().
> 
> But Class::flags isn't in the reserved area.
> 
> > There's at least one more coming down the pipeline.
> 
> What bug? Which fields?

efaust, can you elaborate on this?
Assignee: nobody → nfroyd
Flags: needinfo?(efaustbmo)
Keywords: leave-open
(In reply to Nathan Froyd (:froydnj) from comment #7)
> Leaving open for further trimming of JSClass.
> 
> (In reply to Jason Orendorff [:jorendorff] from comment #3)
> > (In reply to Eric Faust [:efaust] from comment #2)
> > > They sure do! See Class::offsetOfFlags().
> > 
> > But Class::flags isn't in the reserved area.
> > 
> > > There's at least one more coming down the pipeline.
> > 
> > What bug? Which fields?
> 
> efaust, can you elaborate on this?

Part of bug 966518 will add Class::offsetOfProxyHandler(), which will be stored in the reserved fields. This is used in the JIT where our notion of "types" is a little hazy, and so we don't really care about this distinction.
Flags: needinfo?(efaustbmo)
Any more to be done here, froydnj?
Flags: needinfo?(nfroyd)
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Any more to be done here, froydnj?

I still think some sort of split into "classes that need all these extra members" and "classes that don't", plus appropriate fixups for the JITs, would be helpful.
Flags: needinfo?(nfroyd)
The leave-open keyword is there and there is no activity for 6 months.
:froydnj, maybe it's time to close this bug?
Flags: needinfo?(nfroyd)
Sure, I guess.
Status: NEW → RESOLVED
Closed: 11 months ago
Flags: needinfo?(nfroyd)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.