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.

Comment on attachment 8738045 [details] [diff] [review] (part 1) - Rename js::Class::ops as oOps Review of attachment 8738045 [details] [diff] [review]: ----------------------------------------------------------------- APPROVED.

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.

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.

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.)

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

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.

(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.

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.

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.

(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.

Oops, make that http://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/annotations.js#71 Line numbers were based on my local copy, sorry.

(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?

Updated to add a second JIT codegen change, fortunately one that's very similar to the first and so I could do it myself.

(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.

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!

Comment on attachment 8738433 [details] [diff] [review] (part 3) - Codegen Canceling review. This was rolled into a new part 2.

> 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/integration/mozilla-inbound/rev/8246956e2368cc19f4b731ca3a2daca57bcb27f1 Bug 1261723 (part 1) - Rename js::Class::ops as oOps. r=efaust. https://hg.mozilla.org/integration/mozilla-inbound/rev/aa88b0d0cd4adf9e2fde0288a0347b91af32c69a Bug 1261723 (part 2) - Separate class ops from js::Class. code=njn,h4writer. r=efaust,bz.

https://hg.mozilla.org/mozilla-central/rev/8246956e2368 https://hg.mozilla.org/mozilla-central/rev/aa88b0d0cd4a