1118559 - Asking if a slot is aliased is confusing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

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.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: 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.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[Luke Wagner [:luke]]

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.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: 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

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1831405086dc

Comment 7

[Jan de Mooij [:jandem]]

This was a ~10% win or so on Octane-pdfjs on AWFY. Nice :)

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1831405086dc

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89b21469facd

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Clearing tracking requests.