Closed Bug 1642708 Opened 8 months ago Closed 7 months ago

Remove BinAST prototype

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

Attachments

(6 files, 2 obsolete files)

The BinAST code in tree is a prototype that is not practical to ship in its current form and interferes with ongoing work. Until the ongoing SmooshMonkey and Stencil work ships, we should remove the BinAST prototype from tree to avoid wasting time maintaining an intermediate form.

Any future BinAST work will likely be based on the upcoming SmooshMonkey parser/emitter. The encoder and reference designs continue to live on in https://github.com/binast for the meanwhile.

Code to remove:

  • js/src/frontend/BinAST* and all code guarded by JS_BUILD_BINAST
    • These will all need major rewrites before we ship them
  • js/src/jsapi-test/tests/binast
    • These are mostly auto-generated with a few hand-written tests
  • js/src/jit-test/tests/binast
    • These were auto-generated for normal jit-tests
  • Vendored Rust code used by js/src/frontend/binast
    • These remain on external repositories

Automation to remove:

  • The BinAST tests in SM(p) and SM(cgc) builds
  • The Raptor Rap(inst-b), Rap(inst-b-c) performance tasks

Remove the BinAST tests from following jobs:
- SM(p)
- SM(cgc)

Remove the BinAST performance tests:
- binast-instagram

Skip the WPT BinAST tests

Assignee: nobody → tcampbell
Status: NEW → ASSIGNED

Depends on D77919

Attached file Bug 1642708 - Remove BinAST tests (obsolete) —

Remove directories:
js/src/jsapi-tests/binast/
js/src/jit-test/tests/binast/

Remove additional tests:
js/src/fuzz-tests/testBinASTReader.cpp
js/src/jsapi-tests/testBinASTReader.cpp

Depends on D77920

Also remove js/src/frontend/BinAST* sources.

Depends on D77944

Depends on D77945

Depends on D77946

(CC-ing the persons who worked on BinAST)

Ted, any reason why this bug should be confidential?

Flags: needinfo?(tcampbell)

I'm waiting on a few more discussions to happen and final approval that this is the best approach. I'll make sure the bug is open before any reviews actually start. These patches were to understand the scope of change.

Flags: needinfo?(tcampbell)
Severity: -- → N/A
Priority: -- → P2

We intend to remove the prototype code to reduce development complexity of Bug 1619282 and Bug 1601332. In future we will revisit what we will about BinAST implementation (which would likely be based on the new frontend).

Group: mozilla-employee-confidential
See Also: → binjs-implement
Attachment #9153644 - Attachment is obsolete: true

Removing the jit-tests will need to happen manually (because it is too big for Lando) by removing the following paths:

js/src/jsapi-tests/binast/
js/src/jit-test/tests/binast/

I'll need a sheriff to do this manually once the rest of this lands. This does not need to happen at the same time though.

Marking leave-open until Comment 10 is handled.

Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad613fa94a83
Stop running BinAST tests in automation r=perftest-reviewers,Bebe,sfink
https://hg.mozilla.org/integration/autoland/rev/1c245d4e8244
Disable BinAST prototype r=arai
https://hg.mozilla.org/integration/autoland/rev/bcc2728b5ca5
Remove code guarded by JS_BUILD_BINAST r=arai
https://hg.mozilla.org/integration/autoland/rev/d210a60ad435
Remove more references to BinAST code r=arai
https://hg.mozilla.org/integration/autoland/rev/d307b00c7e1b
Remove BinAST Rust modules r=arai,emilio

The dom staticpref include in nsJSUtils.h ends up being used in other places. This was regressed by Bug 1508306, but I'll put the header back in this stack so this can be landed.

Flags: needinfo?(tcampbell)
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ea7e854f8c7
Stop running BinAST tests in automation r=perftest-reviewers,Bebe,sfink
https://hg.mozilla.org/integration/autoland/rev/37322ca8d1ed
Disable BinAST prototype r=arai
https://hg.mozilla.org/integration/autoland/rev/2035099033f2
Remove code guarded by JS_BUILD_BINAST r=arai
https://hg.mozilla.org/integration/autoland/rev/0a60834b51a7
Remove more references to BinAST code r=arai
https://hg.mozilla.org/integration/autoland/rev/36790714faf2
Remove BinAST Rust modules r=arai,emilio

Here is the patch to remove the jit-test. It removes the follow directories:

  • js/src/jsapi-tests/binast/
  • js/src/jit-test/tests/binast/

You should be able to r+ this on bugzilla and we will have a sheriff graft from try to autoland.

Flags: needinfo?(arai.unmht)
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2ce549cea27
Add missing StaticPrefs_dom.h include in DocumentLoadListener r=necko-reviewers,nika,valentin
Attachment #9157426 - Attachment is obsolete: true

Will leave the StaticPrefs_dom.h header alone. Too many accidental uses have slipped into the unified builds depending on chunking.

Flags: needinfo?(tcampbell)
Comment on attachment 9157489 [details]
Bug 1642708 - Remove generated BinAST tests

sorry for the delay.
Flags: needinfo?(arai.unmht)
Attachment #9157489 - Flags: review+
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d98c157b7e08
Remove generated BinAST tests. r=arai CLOSED TREE

After this last patch merges to m-c, this issue can be closed out.

Keywords: leave-open
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
You need to log in before you can comment on or make changes to this bug.