The default bug view has changed. See this FAQ.

Make SuperProperty work in the JITs

NEW
Assigned to

Status

()

Core
JavaScript Engine: JIT
P5
normal
2 years ago
6 months ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8708576 [details] [diff] [review]
Part 0: Allow hop-counting through eval scopes in frontend.

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
Status: NEW → ASSIGNED
Attachment #8708576 - Flags: review?(jorendorff)
(Assignee)

Comment 2

a year ago
Created attachment 8708578 [details] [diff] [review]
Part 1: Rework JSOP_SUPERBASE to take hops
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+

Updated

6 months ago
Assignee: efaustbmo → nobody
Status: ASSIGNED → NEW
Priority: -- → P5

Updated

6 months ago
Assignee: nobody → efaustbmo
You need to log in before you can comment on or make changes to this bug.