Implement some BaselineCodeGen methods

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(17 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

5 months ago
No description provided.
Assignee

Comment 3

5 months ago

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

Assignee

Comment 4

5 months ago

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

Assignee

Comment 8

5 months ago

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

Assignee

Comment 9

5 months ago

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

Depends on D17939

Comment 12

5 months ago
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
Assignee

Updated

5 months ago
Keywords: leave-open

Comment 14

5 months ago
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 callback
promise_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)

Comment 16

4 months ago
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

Comment 17

4 months ago
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
Assignee

Comment 18

4 months ago

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)

Comment 22

4 months ago
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

Comment 24

4 months ago
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

Comment 26

4 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c6fcec3f2b2
part 9 - Update framePushed after Baseline VM call. r=tcampbell
Assignee

Updated

4 months ago
Depends on: 1526947

Comment 30

3 months ago
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
Assignee

Comment 32

3 months ago

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.

Comment 35

3 months ago
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
Assignee

Comment 36

3 months ago

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

Keywords: leave-open

Comment 37

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.