Open Bug 987807 Opened 6 years ago Updated 6 years ago

Don't create Call objects as non-singletons, then mutate them to be singletons

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

REOPENED
mozilla31

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

Generating different bytecode for call objects that are singletons, and for call objects that aren't, is mildly faster -- no need to pass an extra argument for the new-object call, less register pressure.  Also this avoids needless extra TypeObject allocations by just getting it right from the start.

Shu, I'm kind of cargo-culting the parallel portion of this -- tell me if I'm doing something wrong there.  :-)  Since I'm not doing anything functionally different from before, I'm guessing I can generate the same code as before for the run-once parallel case.
Attached patch PatchSplinter Review
Attachment #8396506 - Flags: review?(terrence)
Attachment #8396506 - Flags: review?(shu)
Duplicate of this bug: 987805
Comment on attachment 8396506 [details] [diff] [review]
Patch

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

r=me

::: js/src/jit/CodeGenerator.cpp
@@ +3569,3 @@
>  #ifdef JSGC_GENERATIONAL
> +    if (ins->templateObject()->hasDynamicSlots())
> +    {

{ on previous line.

::: js/src/vm/ScopeObject.cpp
@@ +141,4 @@
>  {
>      gc::AllocKind kind = gc::GetGCObjectKind(shape->numFixedSlots());
>      JS_ASSERT(CanBeFinalizedInBackground(kind, &CallObject::class_));
>      kind = gc::GetBackgroundAllocKind(kind);

Please also JS_ASSERT(!type->isSingleton()) here.

::: js/src/vm/ScopeObject.h
@@ +240,5 @@
> +    /*
> +     * Construct a bare-bones call object given a shape, type, and slots
> +     * pointer.  The call object might or might not have singleton type,
> +     * depending on what |type| is.  The call object must be further
> +     * initialized to be usable.

This is not actually true: Lowering checks if type->isSingleton() and generates an MNewSingletonCallObject in that case. This case is only called by MNewCallObject which should now never be a singleton.
Attachment #8396506 - Flags: review?(terrence) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/539b1a125f71

Mildly curious what (if any) perf benefits are induced by this change, probably none to little, but you never know.  We should find out soon.  :-)
https://hg.mozilla.org/mozilla-central/rev/539b1a125f71
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8396506 [details] [diff] [review]
Patch

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

I'm going to NI Brian and ask him why we still need to be in the VM for setting singleton types.

::: js/src/jit/ParallelSafetyAnalysis.cpp
@@ +536,5 @@
> +ParallelSafetyVisitor::visitNewRunOnceCallObject(MNewRunOnceCallObject *ins)
> +{
> +    replace(ins, MNewCallObjectPar::New(alloc(), ForkJoinContext(), ins));
> +    return true;
> +}

This is not safe *if* setting singleton types still requires to be in VM land. See ParallelSafetyAnalysis::visitLambda.

That said, I'm not sure anymore from reading code why setting singleton types requires us to be in the VM. The template object should already have a singleton type, so there should never be a hash miss. Why can't we just set the type from the template object in the inline masm.initGCThing?
Attachment #8396506 - Flags: review?(shu)
See above comment.
Status: RESOLVED → REOPENED
Flags: needinfo?(bhackett1024)
Resolution: FIXED → ---
I suppose we don't need to be in the VM for setting singleton types.  I'm confused about the point of this bug though, as comment 0 is talking about performance considerations which are really irrelevant for run once call objects, since they should only be created once.
Flags: needinfo?(bhackett1024)
The perf parts were only a side benefit.  Getting rid of the use of setSingletonType and assigning the correct TypeObject from birth was the main goal.  Although, I think this code is now substantially more readable than it was when it had to handle both singleton and non-singleton call objects in the same code path.  Getting rid of the passing in of a null TypeObject handle and hoping things would work just so so that it wouldn't actually be used, and not passing in a script for the sole purpose of determining run-onceness (when such is a static property of the script), also sits far better with me than the previous code.
Okay, if we don't need to be in VM to set singleton types, your PJS changes are fine. I'll file a separate bug to stop going into the VM every time we need to set singleton types.
You need to log in before you can comment on or make changes to this bug.