Closed Bug 1118559 Opened 8 years ago Closed 8 years ago

Asking if a slot is aliased is confusing


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox36 --- fixed
firefox37 --- fixed


(Reporter: shu, Assigned: shu)




(2 files)

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++)

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
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 :)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8546970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Clearing tracking requests.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.