Closed Bug 1522837 Opened 6 years ago Closed 6 years ago

Implement some BaselineCodeGen methods

Categories

(Core :: JavaScript Engine: JIT, enhancement, P2)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(17 files)

Bug 1522837 part 1 - Implement loadScript, emitInitializeLocals, storeFrameSizeAndPushDescriptor for BaselineInterpreterHandler. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 2 - Implement subtractScriptSlotsSize, loadGlobalLexicalEnvironment, loadGlobalThisValue, pushScriptArg. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 3 - Refactor JSOP_OBJECT in BaselineCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 4 - Implement JSOP_CALLSITEOBJ in BaselineInterpreterCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 5 - Implement JSOP_IMPORTMETA in BaselineInterpreterCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 6 - Implement JSOP_BUILTINPROTO in BaselineInterpreterCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 7 - Implement JSOP_NEWARRAY_COPYONWRITE in BaselineInterpreterCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 8 - Implement JSOP_GETIMPORT in BaselineInterpreterCodeGen. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 9 - Update framePushed after Baseline VM call. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 10 - Add interpreter fields to BaselineFrame. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 11 - Implement some simple ops in BaselineInterpreterCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 4.5 - Disable buffer-full-inspect-buffer-during-callback.html WPT test. r?jgraham!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 12 - Support JSOP_NEWARRAY and JSOP_INITELEM_ARRAY in BaselineCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 13 - Implement emitFormalArgAccess in BaselineCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 14 - Implement JSOP_ENVCALLEE, JSOP_NEWTARGET and JSOP_CHECKLEXICAL in BaselineInterpreterCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 15 - Implement aliased var ops in BaselineInterpreterCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1522837 part 16 - Implement pushUint8BytecodeOperandArg and pushUint16BytecodeOperandArg in BaselineInterpreterCodeGen. r?djvj!
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

This also removes computeFullFrameSize because we don't really need it.

This adds js::SingletonObjectLiteralOperation and calls it from both the
interpreter and Baseline. The Baseline compiler still has a fast path for the
cloning-not-necessary case.

Depends on D17645

This is just a VM call in the interpreter. We could optimize this with an IC or
inline path if it ever becomes a problem.

Depends on D17934

Eventually this op could use an IC or some frontend/bytecode refactoring to make
it faster in the interpreter. For now following the C++ interpreter is the
simplest solution though.

Depends on D17938

The Baseline compiler apparently doesn't depend on this but the interpreter needs
framePushed to be correct.

Depends on D17939

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3eeb76f0f5d8
part 1 - Implement loadScript, emitInitializeLocals, storeFrameSizeAndPushDescriptor for BaselineInterpreterHandler. r=djvj
https://hg.mozilla.org/integration/autoland/rev/893db1319f23
part 2 - Implement subtractScriptSlotsSize, loadGlobalLexicalEnvironment, loadGlobalThisValue, pushScriptArg. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/2d7be3f59c9d
part 3 - Refactor JSOP_OBJECT in BaselineCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/814aa7e78267
part 4 - Implement JSOP_CALLSITEOBJ in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/877d383189f4
part 5 - Implement JSOP_IMPORTMETA in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8fb361192757
part 6 - Implement JSOP_BUILTINPROTO in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/47c7f936579b
part 7 - Implement JSOP_NEWARRAY_COPYONWRITE in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/2bd187c04fd9
part 8 - Implement JSOP_GETIMPORT in BaselineInterpreterCodeGen. r=tcampbell
Keywords: leave-open
Backout by btara@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/26db1cffe8de
Backed out 8 changesets for buffer-full-inspect-buffer-during-callback.html failures a=backout

Backed out 8 changesets (Bug 1522837) for buffer-full-inspect-buffer-during-callback.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Copt%2Cweb%2Cplatform%2Ctests%2Cwith%2Ce10s%2Ctest-linux32%2Fopt-web-platform-tests-e10s-11%2Cw-e10s%28wpt11%29&fromchange=ce681d2ac83455eee8057d279b7d6f3384bfbef1&tochange=f4893bab56d5eeb0b468329d885f29eeb51d7aa4&group_state=expanded

Backout link: https://hg.mozilla.org/mozilla-central/rev/26db1cffe8de98af57fabb6db36401cf5cb393b7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225543445&repo=autoland&lineNumber=7745

[task 2019-02-01T22:35:02.991Z] 22:35:02 INFO - TEST-START | /resource-timing/buffer-full-inspect-buffer-during-callback.html
[task 2019-02-01T22:35:07.838Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: too much recursion
[task 2019-02-01T22:35:07.838Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 10: too much recursion
[task 2019-02-01T22:35:07.840Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resources/testharness.js, line 2881: Error: regexp too big
[task 2019-02-01T22:35:07.842Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:07.847Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:07.847Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:07.847Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:07.848Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:07.850Z] 22:35:07 INFO - PID 15808 | JavaScript error: http://web-platform.test:8000/resource-timing/resources/buffer-full-utilities.js, line 12: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
...
...
[task 2019-02-01T22:35:09.247Z] 22:35:09 INFO - PID 15808 | JavaScript error: , line 0: uncaught exception: Error: assert_equals: A sync request was not added to the primary buffer just yet, because it is full expected 1276 but got 1275
[task 2019-02-01T22:35:09.397Z] 22:35:09 INFO - Closing window 23622320129
[task 2019-02-01T22:35:09.434Z] 22:35:09 INFO -
[task 2019-02-01T22:35:09.434Z] 22:35:09 INFO - TEST-UNEXPECTED-FAIL | /resource-timing/buffer-full-inspect-buffer-during-callback.html | Test that entries in the secondary buffer are not exposed during the callback and before they are copied to the primary buffer - assert_equals: All 4 entries should be stored in resource timing buffer. expected 1277 but got 1276
[task 2019-02-01T22:35:09.434Z] 22:35:09 INFO - testThatBufferContainsTheRightResources@http://web-platform.test:8000/resource-timing/buffer-full-inspect-buffer-during-callback.html:40:5
[task 2019-02-01T22:35:09.434Z] 22:35:09 INFO - @http://web-platform.test:8000/resource-timing/buffer-full-inspect-buffer-during-callback.html:52:5
[task 2019-02-01T22:35:09.435Z] 22:35:09 INFO - asyncTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1566:20
[task 2019-02-01T22:35:09.435Z] 22:35:09 INFO - promise_test/tests.promise_tests</<@http://web-platform.test:8000/resources/testharness.js:591:31
[task 2019-02-01T22:35:09.436Z] 22:35:09 INFO - promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:590:20
[task 2019-02-01T22:35:09.436Z] 22:35:09 INFO - promise callbackpromise_test@http://web-platform.test:8000/resources/testharness.js:589:31
[task 2019-02-01T22:35:09.436Z] 22:35:09 INFO - @http://web-platform.test:8000/resource-timing/buffer-full-inspect-buffer-during-callback.html:48:1
[task 2019-02-01T22:35:09.437Z] 22:35:09 INFO - TEST-INFO | expected TIMEOUT
[task 2019-02-01T22:35:09.437Z] 22:35:09 INFO - TEST-ERROR | /resource-timing/buffer-full-inspect-buffer-during-callback.html | took 6441ms
[task 2019-02-01T22:35:09.457Z] 22:35:09 INFO - PID 15808 | 1549060509450 Marionette INFO Stopped listening on port 2828
[task 2019-02-01T22:35:09.559Z] 22:35:09 INFO - PID 15808 | [Parent 15808, Gecko_IOThread] WARNING: pipe error (87): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 357
[task 2019-02-01T22:35:09.560Z] 22:35:09 INFO - PID 15808 | [Parent 15808, Gecko_IOThread] WARNING: pipe error (86): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 357
[task 2019-02-01T22:35:09.999Z] 22:35:09 INFO - Browser exited with return code 0
[task 2019-02-01T22:35:09.999Z] 22:35:09 INFO - PROCESS LEAKS None
[task 2019-02-01T22:35:10.001Z] 22:35:10 INFO - Closing logging queue

Flags: needinfo?(jdemooij)
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9e0cc55d71a
part 1 - Implement loadScript, emitInitializeLocals, storeFrameSizeAndPushDescriptor for BaselineInterpreterHandler. r=djvj
https://hg.mozilla.org/integration/autoland/rev/aea9bf1be0da
part 2 - Implement subtractScriptSlotsSize, loadGlobalLexicalEnvironment, loadGlobalThisValue, pushScriptArg. r=tcampbell
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56d070f54c69
part 3 - Refactor JSOP_OBJECT in BaselineCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8d112cd492e7
part 4 - Implement JSOP_CALLSITEOBJ in BaselineInterpreterCodeGen. r=tcampbell

Hi James, with part 5 here I get the following perma-orange, only on Linux32 opt: resource-timing/buffer-full-inspect-buffer-during-callback.html

I can repro on Try with an even smaller patch that's a pretty trivial interpreter refactoring. I think it has something to do with stack frame size because that test does hit overrecursion.

I think we are expecting a timeout here and that's what we get in a green run: https://searchfox.org/mozilla-central/source/testing/web-platform/meta/resource-timing/buffer-full-inspect-buffer-during-callback.html.ini

A green run has this:

[task 2019-02-02T12:28:59.369Z] 12:28:59 INFO - TEST-ERROR | /resource-timing/buffer-full-inspect-buffer-during-callback.html | took 10382ms

An orange run has:

[task 2019-02-02T12:26:56.843Z] 12:26:56 INFO - TEST-INFO | expected TIMEOUT
[task 2019-02-02T12:26:56.843Z] 12:26:56 INFO - TEST-ERROR | /resource-timing/buffer-full-inspect-buffer-during-callback.html | took 7318ms

If the timeout is 10 seconds that might make sense. Can we disable this test? It looks pretty fishy (timeout, overrecursion errors in the log, etc).

Flags: needinfo?(jdemooij) → needinfo?(james)

It sounds like the test should be fixed. I don't object to disabling it in the short term, but if you do so please file a bug in the appropriate component so that the engineers working on resource timing are able to investigate and fix the issue.

Flags: needinfo?(james)
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/988b11f87ec2
part 4.5 - Disable buffer-full-inspect-buffer-during-callback.html WPT test. r=jgraham
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf42d78396ab
part 5 - Implement JSOP_IMPORTMETA in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/62202236497f
part 6 - Implement JSOP_BUILTINPROTO in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/1ef95148918d
part 7 - Implement JSOP_NEWARRAY_COPYONWRITE in BaselineInterpreterCodeGen. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/9efa9bce49d3
part 8 - Implement JSOP_GETIMPORT in BaselineInterpreterCodeGen. r=tcampbell
Depends on: 1525838
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c6fcec3f2b2
part 9 - Update framePushed after Baseline VM call. r=tcampbell
Depends on: 1526947
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb6108540cef
part 10 - Add interpreter fields to BaselineFrame. r=djvj
https://hg.mozilla.org/integration/autoland/rev/7a376bfac6de
part 11 - Implement some simple ops in BaselineInterpreterCodeGen. r=djvj
https://hg.mozilla.org/integration/autoland/rev/cbba4888990e
part 12 - Support JSOP_NEWARRAY and JSOP_INITELEM_ARRAY in BaselineCodeGen. r=djvj
https://hg.mozilla.org/integration/autoland/rev/0360f25e31db
part 13 - Implement emitFormalArgAccess in BaselineCodeGen. r=djvj

The JSOP_NEWTARGET code for non-arrow functions now uses cmov instead of an if-else. This is
a bit simpler (especially for the interpreter) and shorter and I didn't see any difference
in performance in some Baseline new.target micro-benchmarks.

Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91117189b778
part 14 - Implement JSOP_ENVCALLEE, JSOP_NEWTARGET and JSOP_CHECKLEXICAL in BaselineInterpreterCodeGen. r=djvj
https://hg.mozilla.org/integration/autoland/rev/9b45c3cf37c6
part 15 - Implement aliased var ops in BaselineInterpreterCodeGen. r=djvj
https://hg.mozilla.org/integration/autoland/rev/eb4c19482386
part 16 - Implement pushUint8BytecodeOperandArg and pushUint16BytecodeOperandArg in BaselineInterpreterCodeGen. r=djvj

I think that's enough patches for this bug. We can continue in a new one.

Keywords: leave-open

https://hg.mozilla.org/mozilla-central/rev/91117189b778
https://hg.mozilla.org/mozilla-central/rev/9b45c3cf37c6
https://hg.mozilla.org/mozilla-central/rev/eb4c19482386

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: