Closed Bug 1261723 Opened 4 years ago Closed 4 years ago

Reduce static data sizes by separating class ops from js::Class

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 files, 1 obsolete file)

Bug 1259194, bug 1260984, and bug 1261720 moved ObjectOps, ClassSpec, and ClassExtenion out of js::Class. This bug is about doing the same with the remaining function pointers. Because those function pointers are not currently within a sub-structure, I'll have to introduce one, which I'm planning to name js::ClassOps.

This will give a static data reduction of about 115 KiB.

After this change, the only non-pointer fields in js::Class will be |name| and |flags|, which seems reasonable because |name| is always non-null and |flags| is mostly non-zero.

(BTW, I'm doing these changes close together partly because I want the static data reductions, but also so that the API breakage occurs all at once.)
If you're about to suggest I use |objectOps| instead of |oOps|... I chose the
latter because the number of uses of this field are small and only in two
places, and a longer name would require much uglier formatting of the getters
in js::Class.
Attachment #8738045 - Flags: review?(efaustbmo)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8738045 [details] [diff] [review]
(part 1) - Rename js::Class::ops as oOps

Review of attachment 8738045 [details] [diff] [review]:
-----------------------------------------------------------------

APPROVED.
Attachment #8738045 - Flags: review?(efaustbmo) → review+
efaust: please review the js/ parts. Apologies for the prodigious boringness of
the changes.

bz: please review the dom/ parts.

jorendorff: any feedback on the basic design would be welcome.

Thank you.
Attachment #8738064 - Flags: review?(efaustbmo)
Attachment #8738064 - Flags: review?(bzbarsky)
Attachment #8738064 - Flags: feedback?(jorendorff)
Hannes: would you mind helping me with one JIT change that is required for part 2?

What the patch does is pull the operations (addProperty .. trace) out of js::Class into a new struct js::ClassOps, and changes js::Class to have a pointer to a js::ClassOps struct.

There's one function in the JIT, CodeGenerator::visitIsCallable(), which needs changing. The condition has gone from this:

> is<JSFunction>() || getClass()->call

to this:

> is<JSFunction>() || (getClass()->cOps && getClass()->cOps->call)

I'm not familar with MacroAssembler but I assume you are, and implementing this change will only take you a short time. Thanks you.

BTW, jit-test/tests/asm.js/testSIMD.js is the one jit-test that fails with the current MOZ_CRASH() in that function.
Flags: needinfo?(hv1989)
Comment on attachment 8738064 [details] [diff] [review]
(part 2) - Separating class ops from js::Class

Review of attachment 8738064 [details] [diff] [review]:
-----------------------------------------------------------------

The design looks good to me, but (believe it or not) I'm a little worried about performance.

Terrence, this patch adds a level of indirection to get to the Class::trace() and finalize() hooks. CallTraceHook is hot, right? Will this hurt GC performance?

(The other hooks are mostly touched by the interpreter and jit compilation; once we're running jitcode with ICs populated, we leave them alone. It should be fine.)
Attachment #8738064 - Flags: feedback?(terrence)
Attachment #8738064 - Flags: feedback?(jorendorff)
Attachment #8738064 - Flags: feedback+
Comment on attachment 8738064 [details] [diff] [review]
(part 2) - Separating class ops from js::Class

>+++ b/dom/bindings/Codegen.py

Hmm.  So many relocations.... :(  I'm hoping the startup perf folks don't complain too much.

It really may be worth looking into finding the nsWrapperCache via pointer arithmetic, so we could share the finalize and addProperty hooks.  Then we could share the class ops across most binding implementations.

>+++ b/dom/bindings/SimpleGlobalObject.cpp

So reading this now, I have a few questions (about preexisting code, I know):

1)  Why do we even need SimpleGlobal_enumerate and SimpleGlobal_resolve?  Can we not just use JS_EnumerateStandardClasses and JS_ResolveStandardClass directly?

2)  Does this not need a mayresolve hook to go with its resolve hook?

Followup bug, please; I can take that one.

>+++ b/dom/indexedDB/ActorsParent.cpp

There seem to be several instances of class ops in the tree (here, and in netwerk/base/ProxyAutoConfig.cpp at least) that have these exact class ops: all null except JS_GlobalObjectTraceHook.  Might be worth sharing, maybe.

r=me
Attachment #8738064 - Flags: review?(bzbarsky) → review+
Comment on attachment 8738064 [details] [diff] [review]
(part 2) - Separating class ops from js::Class

Review of attachment 8738064 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, the trace and finalize hooks are quite hot. Certainly hot enough to cache the relevant data structures locally, but probably not hot enough that they'll be seriously impacted by hitting the cache twice? I mean, it's just impossible to tell without vTune and a couple hours of tedious testing. I think it's probably fine, as long as it passes muster elsewhere.
Attachment #8738064 - Flags: feedback?(terrence) → feedback+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Hannes: would you mind helping me with one JIT change that is required for
> part 2?
> 
> What the patch does is pull the operations (addProperty .. trace) out of
> js::Class into a new struct js::ClassOps, and changes js::Class to have a
> pointer to a js::ClassOps struct.
> 
> There's one function in the JIT, CodeGenerator::visitIsCallable(), which
> needs changing. The condition has gone from this:
> 
> > is<JSFunction>() || getClass()->call
> 
> to this:
> 
> > is<JSFunction>() || (getClass()->cOps && getClass()->cOps->call)
> 
> I'm not familar with MacroAssembler but I assume you are, and implementing
> this change will only take you a short time. Thanks you.
> 
> BTW, jit-test/tests/asm.js/testSIMD.js is the one jit-test that fails with
> the current MOZ_CRASH() in that function.

Sure! I'll do it first thing tomorrow.
> Hmm.  So many relocations.... :(  I'm hoping the startup perf folks don't complain too much.

Given the vast number of existing relocations, this is only a drop in the bucket. I'll be surprised if it has a noticeable effect.


> >+++ b/dom/indexedDB/ActorsParent.cpp
> 
> There seem to be several instances of class ops in the tree (here, and in
> netwerk/base/ProxyAutoConfig.cpp at least) that have these exact class ops:
> all null except JS_GlobalObjectTraceHook.  Might be worth sharing, maybe.

I counted those; there were fewer than 10 so I didn't bother.


> Yes, the trace and finalize hooks are quite hot

I just looked through them all and there are many classes that set only trace and/or finalize. I could keep those two hooks in js::Class and put the other 10 in js::ClassOps. A rough calculation suggests it would reduce the space saving from 114 KiB to ~95 KiB.
Flags: needinfo?(hv1989)
Attachment #8738433 - Flags: review?(efaustbmo)
Comment on attachment 8738433 [details] [diff] [review]
(part 3) - Codegen

Review of attachment 8738433 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +10486,5 @@
>      masm.bind(&notFunction);
> +    masm.branchPtr(Assembler::NonZero, Address(output, offsetof(js::Class, cOps)), ImmPtr(nullptr), &hasCOps);
> +    masm.move32(Imm32(0), output);
> +    masm.jump(&done);
> +        

Remove space!
(In reply to Hannes Verschore [:h4writer] from comment #10)
> Created attachment 8738433 [details] [diff] [review]
> (part 3) - Codegen

Lovely! Thank you for the code.
Whiteboard: [MemShrink] → [MemShrink:P2]
sfink: this patch somehow introduces 968 GC hazards: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff5884e098ac&selectedJob=19111076

That seems improbable. I suspect part 2 -- which pulls a bunch of hooks out of js::Class into a new struct js::ClassOps -- is somehow screwing with the analysis. Any suggestions? Thank you.
Flags: needinfo?(sphink)
(In reply to Nicholas Nethercote [:njn] from comment #13)
> sfink: this patch somehow introduces 968 GC hazards:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ff5884e098ac&selectedJob=19111076
> 
> That seems improbable. I suspect part 2 -- which pulls a bunch of hooks out
> of js::Class into a new struct js::ClassOps -- is somehow screwing with the
> analysis. Any suggestions? Thank you.

Update http://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/annotations.js#75 to change js::Class.trace and js::Class.finalize to whatever class contains them now. Unfortunately, that type name is not given in the hazard output. I should add it.
Flags: needinfo?(sphink)
(In reply to Steve Fink [:sfink:, :s:] from comment #14)
> (In reply to Nicholas Nethercote [:njn] from comment #13)
> > sfink: this patch somehow introduces 968 GC hazards:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ff5884e098ac&selectedJob=19111076
> > 
> > That seems improbable. I suspect part 2 -- which pulls a bunch of hooks out
> > of js::Class into a new struct js::ClassOps -- is somehow screwing with the
> > analysis. Any suggestions? Thank you.
> 
> Update
> http://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/
> annotations.js#75 to change js::Class.trace and js::Class.finalize to
> whatever class contains them now. 

I tried that but it didn't help:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af7c226b5015&selectedJob=19125609

Any other suggestions?
Flags: needinfo?(sphink)
Updated to add a second JIT codegen change, fortunately one that's very similar
to the first and so I could do it myself.
Attachment #8739279 - Flags: review?(efaustbmo)
Attachment #8738064 - Attachment is obsolete: true
Attachment #8738064 - Flags: review?(efaustbmo)
(In reply to Nicholas Nethercote [:njn] from comment #16)
> (In reply to Steve Fink [:sfink:, :s:] from comment #14)
> > (In reply to Nicholas Nethercote [:njn] from comment #13)
> > > sfink: this patch somehow introduces 968 GC hazards:
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=ff5884e098ac&selectedJob=19111076
> > > 
> > > That seems improbable. I suspect part 2 -- which pulls a bunch of hooks out
> > > of js::Class into a new struct js::ClassOps -- is somehow screwing with the
> > > analysis. Any suggestions? Thank you.
> > 
> > Update
> > http://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/
> > annotations.js#75 to change js::Class.trace and js::Class.finalize to
> > whatever class contains them now. 
> 
> I tried that but it didn't help:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=af7c226b5015&selectedJob=19125609
> 
> Any other suggestions?

Oh. The problem is that you're not calling js::ClassOps.trace. You're putting that into a local variable and then calling through that.

Which actually reads a little strangely:

    if (JSTraceOp trace = clasp->trace())
        trace(trc, this);

From the name, I would expect clasp->trace() to do the tracing, not return a trace function. js::Class calls these accessors things like getOpsEnumerate(). Shouldn't this be getOpsTrace() or something? (I haven't looked closely at what's going on, so I don't know for sure that this is the right thing.)

But that wouldn't help the hazard. As I see it, you have two options: use an RAII marker

    if (JSTraceOp trace = clasp->getOpsTrace()) {
        AutoSuppressGCAnalysis nogc(trc->runtime());
        trace(trc, this);
    }

or make trace() actually trace:

   clasp->trace(trc, this);
   ...
   void trace(JSTracer* trc, JSObject* obj) {
       if (cOps && cOps->trace)
           (*cOps->trace)(trc, obj);
   }

I *think* that should enable the analysis to see that you're calling js::ClassOps.trace, so your annotation would then work.
Flags: needinfo?(sphink)
Comment on attachment 8739279 [details] [diff] [review]
(part 2) - Separate class ops from js::Class. code=njn,h4writer

Review of attachment 8739279 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the js/src/ bits with the naming comment addressed.

::: js/public/Class.h
@@ +481,5 @@
> +    const char* name;                                                                     \
> +    uint32_t flags;                                                                       \
> +    const ClassOpsType* cOps;                                                             \
> +                                                                                          \
> +    JSAddPropertyOp    addProperty() const { return cOps ? cOps->addProperty : nullptr; } \

I know you talked to Steve about trace and the hazard, and I haven't yet seen what you picked, but I am also a little dubious of having these getters be the actual operation names. I would prefer something like getPropertyClassOp(), to aid readability and make the "maybe nullptr" aspect clear in naming.

@@ +671,5 @@
> +    JSSetterOp          setProperty;
> +    JSEnumerateOp       enumerate;
> +    JSResolveOp         resolve;
> +    JSMayResolveOp      mayResolve;
> +    JSFinalizeOp        finalize;

I guess we are stuck with these two nearly identical structs because of js::FreeOp :/

@@ +907,5 @@
> +              "ClassOps and JSClassOps must be consistent");
> +static_assert(offsetof(JSClassOps, trace) == offsetof(ClassOps, trace),
> +              "ClassOps and JSClassOps must be consistent");
> +static_assert(sizeof(JSClassOps) == sizeof(ClassOps),
> +              "ClassOps and JSClassOps must be consistent");

Thanks for adding all of these :)

::: js/src/ctypes/CTypes.cpp
@@ +588,5 @@
> +};
> +
> +static const JSClassOps sCClosureClassOps = {
> +  nullptr, nullptr, nullptr, nullptr,
> +  nullptr, nullptr, nullptr, CClosure::Finalize,

hmmm, it occurs to me that if we need to, there may be more to win here by splitting out the non-GC ops from ClassOps yet again. This kind of thing seems to be reasonably common.

::: js/src/gc/Marking.cpp
@@ +1288,5 @@
>  
>          return nullptr;
>      }
>  
> +    trace(trc, obj);

Does this work, per Steve's comments?

::: js/src/jit/CodeGenerator.cpp
@@ +10718,5 @@
> +    masm.jump(&done);
> +
> +    masm.bind(&hasCOps);
> +    masm.cmpPtrSet(Assembler::NonZero, Address(output, offsetof(js::ClassOps, call)),
> +                   ImmPtr(nullptr), output);

Looks great. Thanks, Hannes!
Attachment #8739279 - Flags: review?(efaustbmo) → review+
Comment on attachment 8738433 [details] [diff] [review]
(part 3) - Codegen

Canceling review. This was rolled into a new part 2.
Attachment #8738433 - Flags: review?(efaustbmo)
> I would prefer something like
> getPropertyClassOp(), to aid readability and make the "maybe nullptr" aspect
> clear in naming.

We discussed this on IRC and decided on getAddProperty(), getTrace(), getFinalize(), etc.


> I guess we are stuck with these two nearly identical structs because of
> js::FreeOp :/

Yes :/


> > +static const JSClassOps sCClosureClassOps = {
> > +  nullptr, nullptr, nullptr, nullptr,
> > +  nullptr, nullptr, nullptr, CClosure::Finalize,
> 
> hmmm, it occurs to me that if we need to, there may be more to win here by
> splitting out the non-GC ops from ClassOps yet again. This kind of thing
> seems to be reasonably common.

This wouldn't save much space because it wouldn't benefit any of the DOM bindings classes, which dominate because there are so many of them.


> ::: js/src/gc/Marking.cpp
> @@ +1288,5 @@
> >  
> >          return nullptr;
> >      }
> >  
> > +    trace(trc, obj);
> 
> Does this work, per Steve's comments?

I ended up adding separate methods just for executing the trace and finalize hooks and it satisfied the hazard checker.

----

jorendorff expressed concern about how often |trace| and |finalize| are called,
and whether adding an indirection would hurt speed. I did some measurements. of
the class ops doing a couple of minutes of browsing (gmail, CNN, treeherder,
and I ran Octane).

First, here are the frequencies when cOps was nullptr. This case is no slower
than the old case, so doesn't really matter.

> 112166105 counts:
> (  1) 58646615 (52.3%, 52.3%): cOps no:  resolve
> (  2) 12395148 (11.1%, 63.3%): cOps no:  trace
> (  3) 11414955 (10.2%, 73.5%): cOps no:  getProperty
> (  4) 10685162 ( 9.5%, 83.0%): cOps no:  addProperty
> (  5) 10041311 ( 9.0%, 92.0%): cOps no:  setProperty
> (  6)  7550039 ( 6.7%, 98.7%): cOps no:  finalize
> (  7)   718607 ( 0.6%, 99.4%): cOps no:  enumerate
> (  8)   364321 ( 0.3%, 99.7%): cOps no:  delProperty
> (  9)   349943 ( 0.3%,100.0%): cOps no:  call
> ( 10)        4 ( 0.0%,100.0%): cOps no:  construct

And here are the frequencies when cOps is not nullptr. This case has the extra
indirection:

> 35031185 counts:
> (  1) 18991296 (54.2%, 54.2%): cOps yes: trace
> (  2)  8517371 (24.3%, 78.5%): cOps yes: finalize
> (  3)  4911518 (14.0%, 92.5%): cOps yes: resolve
> (  4)   771122 ( 2.2%, 94.7%): cOps yes: getProperty
> (  5)   619802 ( 1.8%, 96.5%): cOps yes: setProperty
> (  6)   427191 ( 1.2%, 97.7%): cOps yes: call
> (  7)   412812 ( 1.2%, 98.9%): cOps yes: mayResolve
> (  8)   273263 ( 0.8%, 99.7%): cOps yes: addProperty
> (  9)    75123 ( 0.2%, 99.9%): cOps yes: enumerate
> ( 10)    16862 ( 0.0%,100.0%): cOps yes: construct
> ( 11)    12509 ( 0.0%,100.0%): cOps yes: hasInstance
> ( 12)     2316 ( 0.0%,100.0%): cOps yes: delProperty

So trace and finalize are hottest, and resolve is the only other significant
one.

It's still simpler and a bigger space win to keep trace and finalize in
ClassOps. I'll do a Talos run and if it looks ok I'll land it like that.
https://hg.mozilla.org/mozilla-central/rev/8246956e2368
https://hg.mozilla.org/mozilla-central/rev/aa88b0d0cd4a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1264561
You need to log in before you can comment on or make changes to this bug.