1825722 - Handle functions that use only arguments.length better

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

I took a quick look at the remaining arguments object allocations in Speedometer after bug 1825014.

A little surprising, almost half of them are for react-stockcharts. It has various small functions that look like this:

d.domain = function(e){
  return arguments.length?(a=i.call(e,se),f()):a.slice()
}
d.interpolate = function(e){
  return arguments.length?(s=e,f()):s
}
d.range = function(e){
  return arguments.length?(l=u.call(e),f()):l.slice()
}
l.x = function(t){
  return arguments.length?(e="function"===typeof t?t:o(+t),l):e
}

Bug 1825014 added JSOp::ArgumentsLength for ArgumentsLength() in self-hosted code. I wonder if we could emit the same bytecode op for functions that use only arguments.length and have no other uses of arguments.

This only affects the interpreter and Baseline tiers, but because there are a bunch of functions in react-stockcharts like this, it adds up.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Writing down some notes as I look into this:

• Currently we work entirely on the binary assumption: Either we use arguments or we don't. Unfortunately, the arguments binding is weird.
• To do this, we'll need to add a new flag to handle the case where the arguments binding exists, but the only use of it is in arguments.length.
• We may be able to do this in simple cases, but anything requiring flow analysis will have to default to the old pattern; probably the new flag is something along the lines of SawNonLengthUseOfArguments.
• The good news is that we disambiguate argument nodes in the syntax parser, which makes this feasible to do in syntax parsing.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

(Am gently poking this, but don't expect anything soon, and if anyone wants to steal feel free; current patch is only some scaffolding)

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1825722 - Add the ability to check if a name is closed over r?arai

Used later in the stack to disambiguate between use in scope and capture.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1825722 - Simplify logic in declareFunctionArgumentsObject r?arai

This was validated by doing a parallel implementation intially and making
sure that the two implementations both declared and indicated used in
the exact same circumstances.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1825722 - Add an ArgumentsLength parse node r?arai

By adding a separate parse node for arguments.length we will be able to
do bytecode generation differently in some circumstances.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1825722 - Where possible avoid allocating arguments, and use JSOp::ArgumentsLength instead r?arai

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3583a8eef745
Add the ability to check if a name is closed over r=arai
https://hg.mozilla.org/integration/autoland/rev/fc800cb4bf61
Simplify logic in declareFunctionArgumentsObject r=arai
https://hg.mozilla.org/integration/autoland/rev/b44b558fdc20
Add an ArgumentsLength parse node r=arai
https://hg.mozilla.org/integration/autoland/rev/35811c7c71e5
Where possible avoid allocating arguments, and use JSOp::ArgumentsLength instead r=arai

Comment 8

[Cristina Horotan [:chorotan]]

Backed out 4 changesets (bug 1825722) for causing bc failures at browser_parsable_script.js

Backout: https://hg.mozilla.org/integration/autoland/rev/2cb963dfbca603e5a675bd9b47cd6e993d7044fa

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=T4NsUhGwSPeuZaShlHmYpg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=35811c7c71e532ef17270c82ec178c71feb9ffd5

Failure log: https://treeherder.mozilla.org/logviewer?job_id=449148863&repo=autoland&lineNumber=3486

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

facepalm Ah yes. Reflect.parse, which I totally forgot about in my joy of getting this functional!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f290a2ed0b0a
Add the ability to check if a name is closed over r=arai
https://hg.mozilla.org/integration/autoland/rev/f5bbb2b73f3a
Simplify logic in declareFunctionArgumentsObject r=arai
https://hg.mozilla.org/integration/autoland/rev/c562efef7f71
Add an ArgumentsLength parse node r=arai
https://hg.mozilla.org/integration/autoland/rev/773fb9c84f60
Where possible avoid allocating arguments, and use JSOp::ArgumentsLength instead r=arai

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing reftest failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/73af30b099fcf9eb7049251e60bcd3de1ac30a44

Push with failures

Failure log -> REFTEST TEST-UNEXPECTED-FAIL | js/src/tests/non262/regress/argumentsReflect.js | Unknown file:///opt/worker/tasks/task_170933021434901/build/tests/jsreftest/tests/js/src/tests/non262/regress/argumentsReflect.js

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/75e7de1200a4
Backed out 4 changesets for causing bc failures at browser_parsable_script.js on a CLOSED TREE

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2dbfdc41394
Add the ability to check if a name is closed over r=arai
https://hg.mozilla.org/integration/autoland/rev/10b96cddce62
Simplify logic in declareFunctionArgumentsObject r=arai
https://hg.mozilla.org/integration/autoland/rev/847a07f3f127
Add an ArgumentsLength parse node r=arai
https://hg.mozilla.org/integration/autoland/rev/086ffc46a18e
Where possible avoid allocating arguments, and use JSOp::ArgumentsLength instead r=arai

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e2dbfdc41394
https://hg.mozilla.org/mozilla-central/rev/10b96cddce62
https://hg.mozilla.org/mozilla-central/rev/847a07f3f127
https://hg.mozilla.org/mozilla-central/rev/086ffc46a18e