Closed Bug 1106982 Opened 10 years ago Closed 10 years ago

Stop doing script/pc lookup in GetPropertyHelperInline if extraWarnings is disabled

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 1103441 comment 4. This has two parts:

(1) Kill JSOP_GETXPROP (so that we don't have to lookup the pc to check for this case). I think JSOP_GETXPROP was an optimization to avoid doing two scope chain walks in this case:

eval("");
x += 55;

00025:  bindname "x"
00030:  dup
00031:  getxprop "x"
00036:  int8 55
00038:  add
00039:  setname "x"

With all the scope chain optimizations, we don't really have to worry about this anymore: NAME (not GNAME!) ops are rare/slow anyway and we can afford emitting a JSOP_NAME:

00025:  bindname "x"
00030:  name "x"
00035:  int8 55
00037:  add
00038:  setname "x"

(2) We should not do the script/pc lookup if the compartment has extraWarnings disabled.
Attached patch Patch (obsolete) — Splinter Review
As described in comment 0, pretty simple patch.

(JSOP_GETXPROP was not Ion-compiled, another reason to remove it.)
Attachment #8531467 - Flags: review?(jorendorff)
Comment on attachment 8531467 [details] [diff] [review]
Patch

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

I think GETXPROP is necessary for ES6 compliance in some dumb cases. Unfortunately.

Here's a test I think we will fail with the patch:

    var x = "wrong";
    var t = {x: "x"};
    var p = new Proxy(t, {
        has(t, id) {
            print("has");
            var found = id in t;
            delete t[id];
            return found;
        },
        get(t, id) { print("get"); return t[id]; }
    });
    with (p)
        x += " x";
    assertEq(t.x, "undefined x");

Current behavior is correct except I think ES6 says we have to call "has" twice before calling "set"; currently we only call it once.

With the patch, we would set t.x to "wrong x", I think.
Attachment #8531467 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> I think GETXPROP is necessary for ES6 compliance in some dumb cases.
> Unfortunately.

Thanks for catching that. I should have seen this :(

> Current behavior is correct except I think ES6 says we have to call "has"
> twice before calling "set"; currently we only call it once.
> 
> With the patch, we would set t.x to "wrong x", I think.

Yup, confirmed. Any thoughts on what's the right way to implement this and avoid both bugs?

Getting the script location and checking for GETXPROP on all "missing property" getprops, is too much of a performance penalty. Maybe we should have a JSOP_GETNAME-like op that handles this? We could only emit it if there's something weird on the scope chain..
Attached patch Updated patchSplinter Review
This patch just passes a flag when we have a JSOP_GETXPROP. Also added the test from comment 2.

Got r+ from Jason on IRC.
Attachment #8531467 - Attachment is obsolete: true
Attachment #8567216 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b83256f370d0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: