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

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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

Comment 5

[Jeff Walden [:Waldo]]

Also https://hg.mozilla.org/integration/mozilla-inbound/rev/2424dfed0c3c because I am dumb.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/539b1a125f71

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a42016d8daa

Comment 8

[Shu-yu Guo [:shu]]

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?

Comment 9

[Shu-yu Guo [:shu]]

See above comment.

Comment 10

[Brian Hackett [Laid off!]]

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.

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Shu-yu Guo [:shu]]

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.

Comment 13

[André Bargull [:anba]]

No longer valid, singleton types have been removed.