Closed Bug 922270 Opened 6 years ago Closed 6 years ago

Don't construct template objects during IonBuilder

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — 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)
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?
(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.
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 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)
Attached patch patchSplinter Review
Assignee: nobody → bhackett1024
Attachment #812210 - Attachment is obsolete: true
Attachment #816010 - Flags: review?(jdemooij)
Component: JavaScript Engine → JavaScript Engine: JIT
Attachment #816010 - Flags: review?(jdemooij) → review+
This patch regressed Kraken-astar and Kraken-oscillator on AWFY.
(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)
https://hg.mozilla.org/mozilla-central/rev/601fb3354112
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 928464
(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)
Depends on: 937793
Depends on: 989586
You need to log in before you can comment on or make changes to this bug.