Closed Bug 1169745 Opened 6 years ago Closed 4 years ago

Make SuperProperty work in the JITs


(Core :: JavaScript Engine: JIT, defect, P5)




Tracking Status
firefox56 --- fixed


(Reporter: efaust, Assigned: tcampbell)


(Blocks 1 open bug)



(5 files, 3 obsolete files)

No description provided.
We can only tell if we have a dynamic scope corresponding to a StaticEvalObject based on its marked strictness. Currently, we mark that strictness after emitting, which is too late to use in the frontend. Instead, seed it with the initial strictness of the script, and mark it immediately when parsing the directive.
Assignee: nobody → efaustbmo
Attachment #8708576 - Flags: review?(jorendorff)
Attachment #8708578 - Flags: review?(jorendorff)
Comment on attachment 8708576 [details] [diff] [review]
Part 0: Allow hop-counting through eval scopes in frontend.

Review of attachment 8708576 [details] [diff] [review]:

It's your lucky day - I am not suspicious and/or knowledgeable enough about what could go wrong here to bury you in requests for new tests.

I kind of doubt we have solid test coverage of this stuff, though, so please do at least add:

* a simple test where it actually goes off and we emit a GET/SETALIASEDVAR;
* and one with GET/SETGNAME, assuming that is actually possible;
* and one where we *can't* optimize some use of an outer variable, because of some enclosing non-strict scope.

Make sure the tests hit the eval cache, too, to make sure that doesn't break anything (though I don't see how it could).

::: js/src/builtin/Eval.cpp
@@ +395,5 @@
> +        // Seed the strictness. Any directives will set the scope's strictness
> +        // in CompileScript.
> +        if (options.strictOption)
> +            staticScope->setStrict();

No --- we've had two near-identical copies of this code for long enough. Common up at least the parts that appear in your -U8 patch, and as much more as is convenient. Even if there are 23 parameters, it'll still save lines of code.

I mean, what the hell.
Attachment #8708576 - Flags: review?(jorendorff) → review+
Comment on attachment 8708578 [details] [diff] [review]
Part 1: Rework JSOP_SUPERBASE to take hops

Review of attachment 8708578 [details] [diff] [review]:

r=me, fun patch!

::: js/src/vm/Interpreter.cpp
@@ +3896,5 @@
> +    MOZ_ASSERT(callee.allowSuperProperty());
> +    MOZ_ASSERT(callee.nonLazyScript()->needsHomeObject());
> +
> +    const Value& homeObjVal = callee.getExtendedSlot(FunctionExtended::METHOD_HOMEOBJECT_SLOT);
> +    RootedNativeObject &homeObj = rootNativeObject0;

ReservedRooted<NativeObject*>, right? (but if not, style nit: move the & left a space - it's supposed to cuddle the type now).

@@ +3899,5 @@
> +    const Value& homeObjVal = callee.getExtendedSlot(FunctionExtended::METHOD_HOMEOBJECT_SLOT);
> +    RootedNativeObject &homeObj = rootNativeObject0;
> +    homeObj = &homeObjVal.toObject().as<NativeObject>();
> +
> +    RootedObject& superBase = rootObject1;

and here
Attachment #8708578 - Flags: review?(jorendorff) → review+
Assignee: efaustbmo → nobody
Priority: -- → P5
Assignee: nobody → efaustbmo
Assignee: efaustbmo → tcampbell
Comment on attachment 8708576 [details] [diff] [review]
Part 0: Allow hop-counting through eval scopes in frontend.

The frontend rewrite removes the need for this patch.
Attachment #8708576 - Attachment is obsolete: true
Functional patch is up. I'll do a little more rework before review and support for JSOP_SUPERFUN.

As it stands, I'm not sure changing the bytecode helps much so am opting not to do it. I use BaselineFrame::calleeToken to avoid the environment walk in normal (non-eval, non-arrow) cases.
Moving JSOP_SUPERFUN changes to Bug 1169746.
Attachment #8708578 - Attachment is obsolete: true
JSOP_GETPROP_SUPER was missing TI update in interpreter.
Attachment #8875480 - Flags: review?(jdemooij)
Support the JSOP_SUPERBASE opcode in baseline to allow |super.x|.

One optimization is in typical case I use frame.callee to get HomeObject rather than walking the environment.
Attachment #8874039 - Attachment is obsolete: true
Comment on attachment 8875481 [details] [diff] [review]

Allow |super.x| in Baseline.
Attachment #8875481 - Flags: review?(jdemooij)
Attachment #8875480 - Flags: review?(jdemooij) → review+
Comment on attachment 8875481 [details] [diff] [review]

Review of attachment 8875481 [details] [diff] [review]:

Very nice to have the inline path.

::: js/src/jit/BaselineCompiler.cpp
@@ +4163,5 @@
> +        if (si.hasSyntacticEnvironment() && si.scope()->is<FunctionScope>()) {
> +            JSFunction* fn = si.scope()->as<FunctionScope>().canonicalFunction();
> +
> +            if (!fn->isArrow())
> +                break;;

Nit: s/;;/;/

@@ +4168,5 @@
> +        }
> +
> +        // Traverse environment chain
> +        if (si.scope()->hasEnvironment()) {
> +            Address next_addr(reg, EnvironmentObject::offsetOfEnclosingEnvironment());

Nit: nextAddr

@@ +4193,5 @@
> +    // Lookup callee object of environment containing [[ThisValue]]
> +    getThisEnvironmentCallee(scratch);
> +
> +    // Load [[HomeObject]]
> +    Address homeobj_addr(scratch, FunctionExtended::offsetOfMethodHomeObjectSlot());

Nit: homeObjAddr

::: js/src/jit/VMFunctions.cpp
@@ +1804,5 @@
>      return true;
>  }
> +JSObject*
> +HomeObjectSuperBase(JSContext* cx, HandleObject homeObj)

Please move this function to vm/Interpreter.cpp/h and call it in the interpreter for JSOP_SUPERBASE, to avoid duplicating the logic.
Attachment #8875481 - Flags: review?(jdemooij) → review+
Pushed by
JSOP_GETPROP_SUPER should Typescript::Monitor. r=jandem
Support JSOP_SUPERBASE in Baseline. r=jandem

- Attach TypeMonitor_AnyValue IC for supercall TDZ values so they don't VMCall on every run.
- Refactor to add IsConstructorCallPC helper to reduce special cases of JSOP_SUPERCALL.
- Refactor branchIfFunction* helpers to use common helper and add branchIfNotConstructor.
Oops, wrong bug. TODO list for here is to support JSOP_GETPROP_SUPER and friends.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See comment 15, we don't actually support this yet.
Resolution: FIXED → ---
Depends on: 1374462
Turns out the stack-order documentation in vm/Opcodes.h was inconsistent with the interpreter code.

As well, the JOF_TYPESET annotations were missing. These will force typeset locations to be allocated that may not have before, but I haven't had any issues in various try runs this week.
Attachment #8880564 - Flags: review?(jdemooij)
JSOP_GETPROP_SUPER Baseline is ready for review.

I used GetPropIRGenerator to take advantage of all the good work there. I checked the existing stub variants as best I could and included tests where they made sense. I explicitly avoid Proxies for now since |this| behavior is tricky. Native, Unboxed, megamorphic cases should all work.

One area that is a little weak is the SharedIC.cpp changes requiring duplicating DoGetPropFallback due to different arguments and tail-call trampoline needed. Ideas welcome to clean that up (possibly a follow-up patch).
Attachment #8880843 - Flags: review?(jdemooij)
Follow up to patch 0004 with GETELEM_SUPER support.
Attachment #8880902 - Flags: review?(jdemooij)
Comment on attachment 8880564 [details] [diff] [review]

Review of attachment 8880564 [details] [diff] [review]:

Good find.
Attachment #8880564 - Flags: review?(jdemooij) → review+
Comment on attachment 8880843 [details] [diff] [review]

Review of attachment 8880843 [details] [diff] [review]:

This is super nice (no pun intended). Very cool we can support even getters with only minor CacheIR emitter changes.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1951,5 @@
>  #endif
>          break;
> +#if DEBUG
> +      default:
> +        MOZ_CRASH("Unsupported IC");

We usually don't add the default case when it's not needed, to ensure the compiler warns when we add a new enum value. If you want to add it I'd remove the #if.

@@ +2010,5 @@
>          stubKind = CacheIRStubKind::Updated;
>          break;
> +#if DEBUG
> +      default:
> +        MOZ_CRASH("Unsupported IC");

Same here.

::: js/src/jit/CacheIR.cpp
@@ +604,5 @@
>          trackAttached("NativeSlot");
>          return true;
> +      case CanAttachCallGetter: {
> +        // |super.prop| accesses use a |this| value that differs from lookup object
> +        ObjOperandId receiverId = isSuper() ? writer.guardIsObject(getThisValueId())

See bug 1337812. Similar to that, I wonder if it would be nicer to pass the receiver as Maybe<ValOperandId> or something, to make things more explicit. r=me either way.

@@ +994,5 @@
>      if (mode_ == ICState::Mode::Megamorphic)
>          return tryAttachGenericProxy(obj, objId, id, /* handleDOMProxies = */ true);
> +    // The proxy stubs don't currently support |super| access.
> +    if (isSuper())

Shouldn't we move this check before the tryAttachGenericProxy call? We can get here when a class inherits from a proxy?

::: js/src/jit/CacheIR.h
@@ +1136,5 @@
>          MOZ_ASSERT(cacheKind_ == CacheKind::GetElem);
>          return ValOperandId(1);
>      }
> +    ValOperandId getThisValueId() const {

Maybe getReceiverValueId? Or even getSuperReceiverValueId, it's pretty easy to confuse them.
Attachment #8880843 - Flags: review?(jdemooij) → review+
Comment on attachment 8880902 [details] [diff] [review]

Review of attachment 8880902 [details] [diff] [review]:

::: js/src/jit/BaselineIC.h
@@ +415,5 @@
>      // Compiler for this stub kind.
>      class Compiler : public ICStubCompiler {
>        protected:
> +        bool hasReceiver_;

IIRC the previous patch called this hasThis_. We should probably change it to use hasReceiver_, too.
Attachment #8880902 - Flags: review?(jdemooij) → review+
Good catch on the isSuper after megamorphic check. I must have scrambled it up when cleaning up patches.

I'll also s/this/receiver/.

I'll remove the default cases. I had put them in because I forgot one of those cases and didn't notice the warnings.

I'll leave the getXXXValueId vs Maybe<> stuff for now and add a comment to that bug.
Pushed by
Fix vm/Opcodes.h for JSOP_GETPROP_SUPER / JSOP_GETELEM_SUPER. r=jandem
Support JSOP_GETPROP_SUPER in Baseline. r=jandem
Support JSOP_GETELEM_SUPER in Baseline. r=jandem
Looks like this improved add-on manager and system add-on startup by about 8-9% (~10ms on my machine), and some areas of the WebExtension framework by >15%.

This also shows up in talos as about a 10ms improvement on win64 ts_paint opt.
You need to log in before you can comment on or make changes to this bug.