Closed Bug 1558604 Opened 7 months ago Closed 6 months ago

Further Reduce reliance on FunctionBox's function() accessor

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1552316 +++

To support a longer term goal of minimizing the GC interaction with the parser, one stepping-stone is the eventual removal of the JSFunction accessible via the FunctionBox.

That's a longer term goal, but we can already start reducing our reliance on that member.

The args count needs to be set before the LazyScript takes hold of the
functionbox, or else some code that references lazy functions can get the wrong
number of arguments (ie, CloneFunctionObjectIfNotSingleton on a lazy function)

Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6accf00aa7cc
Defer initializaton of JSFunction::nargs until after function parsing is done. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/2fa5845aab8f
Keep interpreted and interpretedLazy on FunctionBox r=jorendorff

Whelp, that was unexpected. Looking.

Flags: needinfo?(mgaudet)

Could you please look also at these failures? https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251640716&repo=autoland&lineNumber=12818

task 2019-06-13T13:09:51.690Z] 13:09:51 INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js | 1404 / 6483 (21%)
[task 2019-06-13T13:09:51.717Z] 13:09:51 INFO - ++DOMWINDOW == 35 (0x7f6855dea000) [pid = 1119] [serial = 2756] [outer = 0x7f6860d73660]
[task 2019-06-13T13:09:51.839Z] 13:09:51 INFO - TEST-INFO | FAILED! uncaught exception: Test262Error: Conforms to NativeFunction Syntax: 'class /* a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / }'.(class / a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / } / k / m / l / ( / m / ) / n / { / o / } / p / })
[task 2019-06-13T13:09:51.840Z] 13:09:51 INFO - JavaScript error: , line 0: uncaught exception: Test262Error: Conforms to NativeFunction Syntax: 'class /
a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / }'.(class / a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / } / k / m / l / ( / m / ) / n / { / o / } / p / })
[task 2019-06-13T13:09:51.882Z] 13:09:51 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js | Unknown :0: uncaught exception: Test262Error: Conforms to NativeFunction Syntax: 'class /
a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / }'.(class / a / A / b / extends / c / B / d / { / e / constructor / f / ( / g / ) / h / { / i / ; / j / } / k / m / l / ( / m / ) / n / { / o / } / p */ }) item 1
[task 2019-06-13T13:09:51.884Z] 13:09:51 INFO - REFTEST INFO | Saved log: START file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js
[task 2019-06-13T13:09:51.884Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
[task 2019-06-13T13:09:51.884Z] 13:09:51 INFO - REFTEST INFO | Saved log: Initializing canvas snapshot
[task 2019-06-13T13:09:51.884Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] AfterOnLoadScripts belatedly entering WaitForTestEnd
[task 2019-06-13T13:09:51.886Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] WaitForTestEnd: Adding listeners
[task 2019-06-13T13:09:51.887Z] 13:09:51 INFO - REFTEST INFO | Saved log: Initializing canvas snapshot
[task 2019-06-13T13:09:51.889Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
[task 2019-06-13T13:09:51.890Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: dispatching MozReftestInvalidate
[task 2019-06-13T13:09:51.893Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
[task 2019-06-13T13:09:51.895Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_SPELL_CHECKS
[task 2019-06-13T13:09:51.899Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
[task 2019-06-13T13:09:51.901Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: APZ flush not required
[task 2019-06-13T13:09:51.902Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
[task 2019-06-13T13:09:51.903Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: Doing sync flush to compositor
[task 2019-06-13T13:09:51.904Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] MakeProgress: Completed
[task 2019-06-13T13:09:51.905Z] 13:09:51 INFO - REFTEST INFO | Saved log: [CONTENT] RecordResult fired
[task 2019-06-13T13:09:51.906Z] 13:09:51 INFO - REFTEST INFO | Saved log: RecordResult fired
[task 2019-06-13T13:09:51.907Z] 13:09:51 INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js
[task 2019-06-13T13:09:51.910Z] 13:09:51 INFO - ++DOMWINDOW == 36 (0x7f6855f6f800) [pid = 1119] [serial = 2757] [outer = 0x7f6860d73660]
[task 2019-06-13T13:09:51.946Z] 13:09:51 INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/built-ins/Function/prototype/toString/class-declaration-implicit-ctor.js

Flags: needinfo?(mgaudet)

Yeah, it's the same test case, just run in the browser vs the shell.

the failing patch is https://hg.mozilla.org/integration/autoland/rev/2fa5845aab8f Keep interpreted and interpretedLazy on FunctionBox r=jorendorff

Investigating now.

Flags: needinfo?(mgaudet)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:mgaudet, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(mgaudet)

Bug 1558604 - Defer initializaton of JSFunction::nargs until after function parsing is done. r=jorendorff; is busted still investigating.

Flags: needinfo?(mgaudet)
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba60dd480690
Keep interpreted and interpretedLazy on FunctionBox r=jorendorff

Well then.

The patch I thought was good... is also bad. And also bad only with BinAST.

Wat.

Flags: needinfo?(mgaudet)

Prelude

My goal right now is to disentangle the creation of JSFunctions from our parser. The current approach is to use FunctionBox to encapsulate all the information gathered during parse, updating the JSFunction only when parsing is done.

The idea here is that once we have all the relevant information stored on the FunctionBox, we can then as a next step, defer the entire creation of the JSFunction to the end of parsing.

I'm struggling mightily with trying to make this work with BinAST. The example case I have right now is the attached patch, however I think I've run into similar problems elsewhere. I won't write those up because I don't have as good a confirmation though.

The problematic patch:

So, the goal of this patch is to take responsiblity for nargs in the FunctionBox. This patch can be recreated in minutes:

  • Add an nargs_ field to FunctionBox; add setter setArgCount and getter nargs.
  • Change all the references to funbox->function()->setArgCount and funbox->function()->setArgCount to instead manipulate the funbox directly
  • In each of the PerHandlerParser<T>::finishFunction synchronize the FunctionBox's arg count with the JSFunction's

This is sufficient to handle non-BinAST code. This works because all manipulation of the FunctionBox's argument counts happens before finishFunction, and after that we've let the JSFunction 'escape' into the wider post-parsing world.

The problem is that in BinAST, it's not clear where the right place to synchronize would be. At early attempt, which -should- have been equivalent to what existed previously takes all setArgCalls in BinAST and immediately synchronizes to the function(); yet this doesn't seem to work.

Depending on where I sync up the nargs, I occasionally even get complaints about the expectedLength -- which I find baffling as I had understood length and nargs() to largely represent different data!

Suffice it to say, I'm at my wits end with this one.

Any help appreciated!

Flags: needinfo?(arai.unmht)

if I add finishFunction-equivalent thing (that called after parsing params and body, both for eager and lazy case) to BinAST,
does it solve the issue?

Depends on: 1563489

I think it would certainly aid understanding to have a similar flow! Thanks Arai!

Attachment #9071385 - Attachment is obsolete: true

The args count needs to be set before the LazyScript takes hold of the
functionbox, or else some code that references lazy functions can get the wrong
number of arguments (ie, CloneFunctionObjectIfNotSingleton on a lazy function)

Flags: needinfo?(arai.unmht)
Blocks: 1567579
Attachment #9076063 - Attachment description: Bug 1558604 - Defer initializaton of JSFunction::nargs until after function parsing is done. r=jorendorff → Bug 1558604 - Defer initialization of JSFunction::nargs until after function parsing is done. r=jorendorff
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7edcc3c47ce
Keep interpreted and interpretedLazy on FunctionBox r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/dbba31c9f096
Defer initialization of JSFunction::nargs until after function parsing is done. r=jorendorff
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Blocks: 1544117
You need to log in before you can comment on or make changes to this bug.