Asking if a slot is aliased is confusing

RESOLVED FIXED in Firefox 36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla37
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
JSScript::varIsAliased is confusingly named. From the name alone, it seems like you can ask if a var slot is aliased. But this doesn't even make sense: vars with slots are not aliased by definition; if they didn't have a slot, then they would live in the CallObject and would not have a slot.

The argument that varIsAliased really expects is the binding index into the script's binding array, not the frame index.

I found this while investigating why "inner" in the snippet below wasn't getting inlined:

function bench() {
  var x = 0;
  function inner() { x++; }
  for (var i = 0; i < 1000000; i++)
    inner();
  print(x);
}

x has bindingIndex = 0, frameIndex = n/a because it's aliased
inner has bindingIndex = 1, frameIndex = 0

Ion then asks JSScript if slot 0 is aliased, and JSScript erroneously returns true.
(Assignee)

Comment 1

3 years ago
Created attachment 8544926 [details] [diff] [review]
Make checking if a slot is aliased less confusing.

Rename varIsAliased to bindingIsAliased and require the caller to pass in a
BindingIter instead of a bare uint32, to make it clear that this is not a slot
number.
Attachment #8544926 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Assignee: nobody → shu
Comment on attachment 8544926 [details] [diff] [review]
Make checking if a slot is aliased less confusing.

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

Much nicer! Thanks.
Attachment #8544926 - Flags: review?(jdemooij) → review+
Btw, please backport the CompileInfo.h change to Aurora. It's a one-liner and (based on the testcase in comment 0) this might affect a lot of code.
Blocks: 1090491
status-firefox36: --- → affected
status-firefox37: --- → affected
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?

Comment 4

3 years ago
Good improvement!  To wit, before Jan's recent work, all variables used to have slots, even the aliased ones, so one could query whether a slot was aliased (which meant the slot was dead space).  This weirdness derives from the original design of storing all variables in the StackFrame and having the CallObject alias the StackFrame.
(Assignee)

Comment 5

3 years ago
Created attachment 8546970 [details] [diff] [review]
Just CompileInfo.h changes

Approval Request Comment
[Feature/regressing bug #]:‎ 1090491
[User impact if declined]: possibly slower JS
[Describe test coverage new/current, TBPL]: pushed to central
[Risks and why]: low, no new feature, just bug fix
[String/UUID change made/needed]: none
Attachment #8546970 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1831405086dc
This was a ~10% win or so on Octane-pdfjs on AWFY. Nice :)
https://hg.mozilla.org/mozilla-central/rev/1831405086dc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8546970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/89b21469facd
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Clearing tracking requests.
tracking-firefox36: ? → ---
tracking-firefox37: ? → ---
Flags: qe-verify-

Updated

3 years ago
Duplicate of this bug: 1112396
You need to log in before you can comment on or make changes to this bug.