Closed Bug 933798 Opened 6 years ago Closed 6 years ago

Don't inhibit name optimizations in try blocks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
As discussed on IRC, there's some unnecessary code that's deoptimizing name accesses in try blocks.

This gets rid of the BINDNAME ops that are slowing down the x86 emulator in bug 933369.
Attachment #825952 - Flags: review?(bhackett1024)
Attachment #825952 - Flags: review?(bhackett1024) → review+
This means there's no reason not to support `let` in lazy-parsed functions, right?  catch-blocks are just like let-blocks, in terms of scoping.
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> This means there's no reason not to support `let` in lazy-parsed functions,
> right?  catch-blocks are just like let-blocks, in terms of scoping.

This patch just deals with the final emitted bytecode and how we optimize name accesses in functions that were lazily parsed.  There's still the issue of handling 'let' blocks when detecting syntax errors during the initial lazy parse.
https://hg.mozilla.org/mozilla-central/rev/495a9c210b91
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 935348
Backed out for causing crashes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/175bebe48034
Status: RESOLVED → REOPENED
Depends on: 934789
Resolution: FIXED → ---
Hm here's the testcase luke posted in bug 934789. I'm pretty sure that's caused by this bug. Brian do you have any ideas offhand before I dive in? It would be great if we didn't have to deoptimize names in try blocks.

(function() {
  function foo() {}

  dis(function bar(e) {
    try {
        (function() { e; });
    }
    catch (e) {
      foo(); // << assertion in dis() here
    }
  });
}());
Flags: needinfo?(bhackett1024)
Target Milestone: mozilla28 → ---
Do make sure those testcases get landed :)
Flags: in-testsuite?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Do make sure those testcases get landed :)

jorendorff already pushed tests :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6e3ca41348
Flags: in-testsuite? → in-testsuite+
Attached patch Patch v2Splinter Review
The old code checked for STMT_TRY and STMT_FINALLY, but STMT_CATCH is the one we care about right? Well, it turns out when we're inside a catch block, the STMT_CATCH StmtInfo has an *outer* STMT_TRY StmtInfo that was used to prevent optimizing name accesses in catch blocks.

This patch checks for STMT_CATCH instead of STMT_TRY/STMT_FINALLY and now passes the two tests that were added.
Attachment #825952 - Attachment is obsolete: true
Attachment #828230 - Flags: review?(bhackett1024)
Flags: needinfo?(bhackett1024)
Depends on: 935491
Depends on: 934997
Depends on: 935006
Depends on: 935322
Depends on: 935331
Comment on attachment 828230 [details] [diff] [review]
Patch v2

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

Sorry for the delay.
Attachment #828230 - Flags: review?(bhackett1024) → review+
Pushed with some extra tests decoder found:

https://hg.mozilla.org/integration/mozilla-inbound/rev/22237babbbf9
https://hg.mozilla.org/mozilla-central/rev/22237babbbf9
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.