Asking if a slot is aliased is confusing

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: shu, Assigned: shu)

Tracking

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(2 attachments)

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.
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: 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.
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.
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?
This was a ~10% win or so on Octane-pdfjs on AWFY. Nice :)
https://hg.mozilla.org/mozilla-central/rev/1831405086dc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8546970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Clearing tracking requests.
Flags: qe-verify-
Duplicate of this bug: 1112396
You need to log in before you can comment on or make changes to this bug.