Closed Bug 1825722 Opened 1 year ago Closed 2 months ago

Handle functions that use only arguments.length better

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
125 Branch
Tracking Status
firefox125 --- fixed

People

(Reporter: jandem, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sp3])

Attachments

(4 files)

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.

Severity: -- → S3
Priority: -- → P3

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.

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

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

Assignee: nobody → mgaudet
Status: NEW → ASSIGNED

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.

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

Pushed by mgaudet@mozilla.com:
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

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

Flags: needinfo?(mgaudet)
Pushed by mgaudet@mozilla.com:
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
Backout by pstanciu@mozilla.com:
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
Pushed by mgaudet@mozilla.com:
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
Flags: needinfo?(mgaudet)
Regressions: 1883837
Blocks: 1883987
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: