Closed
Bug 922270
Opened 11 years ago
Closed 11 years ago
Don't construct template objects during IonBuilder
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file, 1 obsolete file)
48.92 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Currently, IonBuilder constructs template objects which are used for inlining object construction, passing to calls, and so forth. Constructing JS objects requires access to many parts of the VM and shouldn't be performed when running off thread. Instead, the template objects which Ion needs could be obtained from the baseline JIT, which already constructs the objects in some cases and can be extended to provide templates in the remainder. The attached patch does this for [] and {} literals, as well as 'new Foo()' and many of the natives which are inlined using template objects. This patch skips some parallel array related intrinsics, I need a better understanding of how PJS interacts with off thread compilation before knowing how to proceed with that stuff.
Attachment #812210 -
Flags: review?(jdemooij)
Comment 1•11 years ago
|
||
It's a bit unfortunate that we have to allocate objects during Baseline compilation just because Ion may need them later on, especially since most scripts are never Ion compiled at all. I was going to suggest running BytecodeAnalysis on the main thread and doing the allocation there, but that won't work well with inlining of course (we don't know beforehand which scripts we're going to inline). Instead of allocating objects from the GC heap, how hard would it be to do what you suggested somewhere else and pass a "description" of the object to codegen?
Comment 2•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1) > Instead of allocating objects from the GC heap, how hard would it be to do what you > suggested somewhere else and pass a "description" of the object to codegen? We could allocate, say, an ArrayObject and StringObject on the main thread and patch the TypeObject pointers in CodeGenerator::link on the main thread. Or run IonBuilder off-thread, then allocate any template objects on the main thread and then run the rest of compilation off-thread again. I think createThisScriptedSingleton is the most complicated case, but at least for "new String" inlining etc something like this should work.
Assignee | ||
Comment 3•11 years ago
|
||
It certainly could be done the way you describe, but would be considerably more complicated --- we aren't just patching object pointers but need to patch the inline code constructing the object with the right shape/type, need to know the number of slots and the size class of the object and so forth. Baseline already creates template objects for literals, and in the patch it reuses template objects from TypeNewScript. The only new memory use is really the template objects created for a handful of specific natives, and the slot in a couple different baseline caches for optionally holding a template object. This memory should be pretty puny, and I think trying to optimize it now would be a distraction given the scale of the changes required for bug 785905.
Comment 4•11 years ago
|
||
Comment on attachment 812210 [details] [diff] [review] patch Review of attachment 812210 [details] [diff] [review]: ----------------------------------------------------------------- I can't think of a good alternative approach so this is ok for now I think once the comments below are addressed. ::: js/src/jit/BaselineCompiler.cpp @@ +1450,5 @@ > return false; > } > > // Pass base object in R0. > masm.movePtr(ImmGCPtr(templateObject), R0.scratchReg()); We can remove this move + DoNewObject argument and use stub->templateObject() there. @@ +1504,5 @@ > > // Pass base object in R0. > masm.movePtr(ImmGCPtr(templateObject), R0.scratchReg()); > > + ICNewObject_Fallback::Compiler stubCompiler(cx, templateObject); And here. ::: js/src/jit/BaselineIC.cpp @@ +7511,5 @@ > } > > + // Check for natives to which template objects can be attached. This is > + // done to provide templates to Ion for inlining these natives later on. > + RootedObject templateObject(cx); Move this into its own function, so that you can do something like: RootedObject templateObject(cx, TemplateObjectForNative(cx, fun->native(), argc, vp)); @@ +7516,5 @@ > + if (fun->native() == js_Array) { > + size_t count = 0; > + if (argc > 1) > + count = argc; > + else if (argc == 0 && vp[2].isInt32() && vp[2].toInt32() > 0) argc == 1. Also, can we use CallArgs here somehow and pass that to the new function instead of argc + vp? ::: js/src/jit/MCallOptimize.cpp @@ +209,5 @@ > { > uint32_t initLength = 0; > MNewArray::AllocatingBehaviour allocating = MNewArray::NewArray_Unallocating; > > + JSObject *templateObject = inspector->getTemplateObject(pc); Can we have something like: inspector->getTemplateObjectForNative(cx, js_Array) here and turn the is<ArrayObject>() check into an assert? Avoids ending up with array_concat's template object or something. That's probably not a disaster but it seems nicer to avoid it.
Attachment #812210 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•11 years ago
|
||
Assignee: nobody → bhackett1024
Attachment #812210 -
Attachment is obsolete: true
Attachment #816010 -
Flags: review?(jdemooij)
Assignee | ||
Updated•11 years ago
|
Component: JavaScript Engine → JavaScript Engine: JIT
Updated•11 years ago
|
Attachment #816010 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ebfef56fee
Comment 7•11 years ago
|
||
Backed out for various shell failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/af90be985882 https://tbpl.mozilla.org/php/getParsedLog.php?id=29206127&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=29205052&tree=Mozilla-Inbound
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/601fb3354112
Comment 9•11 years ago
|
||
This patch regressed Kraken-astar and Kraken-oscillator on AWFY.
Comment 10•11 years ago
|
||
(In reply to Guilherme Lima from comment #9) > This patch regressed Kraken-astar and Kraken-oscillator on AWFY. Yup, seems to be about 10% each.
Flags: needinfo?(bhackett1024)
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/601fb3354112
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #10) > (In reply to Guilherme Lima from comment #9) > > This patch regressed Kraken-astar and Kraken-oscillator on AWFY. > > Yup, seems to be about 10% each. Filed bug 928464.
Flags: needinfo?(bhackett1024)
You need to log in
before you can comment on or make changes to this bug.
Description
•