Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Playground 2022 #5375

Merged
merged 13 commits into from
Jun 1, 2022
Merged

Playground 2022 #5375

merged 13 commits into from
Jun 1, 2022

Conversation

ryyppy
Copy link
Member

@ryyppy ryyppy commented May 7, 2022

Continuing the work of #5079. Here are the main changes:

  • Rebase on latest master
  • Remove refmt support (not sure what it was, but there are some fundamental issues with Refmt_api and some src/ast_406.ml type mismatches)
  • Improving the repl.js file to allow compilation of different JS bundle versions (by default it will compile the js_compile bundle)

Note on the refmt thing: I guess the only way to re-introduce refmt support would be via a separate bundle. Basically like in the good old BuckleScript playground times.

The lack of refmt support eliminates half of the playground code though, so from a maintenance perspective it's way easier now.

@ryyppy ryyppy mentioned this pull request May 7, 2022
@@ -69,6 +69,8 @@ val printer : (formatter -> t -> unit) ref
val warning_printer : (t -> formatter -> Warnings.t -> unit) ref
(** Hook for intercepting warnings. *)

val formatter_for_warnings : formatter ref
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately i need this interface, otherwise I can't reset the warning buffer between multiple runs

@@ -37,7 +37,9 @@ o $SNAP/unstable/js_compiler.ml: bspack | ./bin/bspack.exe $LTO
flags = -D BROWSER=true -MD -I ml $includes
main = Jsoo_main


o $SNAP/unstable/js_playground_compiler.ml: bspack | ./bin/bspack.exe $LTO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if there's a way to do conditional rules to add the playground snapshot build

Copy link
Member Author

@ryyppy ryyppy May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristianoc compilation performance should be generally good, so running this with every ninja build feels like the right thing to do. Also the jsoo_main entrypoint is identical from a dependency perspective and always has been part of the build process ever since.

Note: The slowest part of building the playground is running jsoo with scripts/repl.js, and we'd probably want to set up a CI job later to automate / test the bundle build process. We don't need to run this compilation on a regular basis since JSOO is quite reliable as long as the ocaml code compiles.

Copy link
Member Author

@ryyppy ryyppy May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here are some benchmarks.

Test instructions:

  • Edit jscomp/snapshot.ninja to enable / disable certain rules.
  • Edit a line in jscomp/ml/location.mli
  • Run config / build

BASELINE: no snapshot rule for any playground

scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/reactDOMRe.cmi
rescript: [2/2] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build  10.22s user 4.53s system 291% cpu 5.057 total

TEST 1: snapshot rule for jsoo_main only

scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/unsafe_this.cmj
rescript: [3/3] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build  11.26s user 4.75s system 310% cpu 5.147 total

TEST 2: snapshot rule for jsoo_main and jsoo_playground_main

scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/flow_parser_reg_test.cmj
rescript: [4/4] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build  12.24s user 4.74s system 268% cpu 6.324 total

TEST 3: snapshot rule for jsoo_playground_main only

scripts/ninja.js config && time scripts/ninja.js build
config for the first time may take a while
rescript: no work to do.
rescript: [983/983] test/flow_parser_reg_test.cmj
rescript: [3/3] ../lib/4.06.1/whole_compiler.ml
POST-PROCESSING-FILE: ../lib/4.06.1/whole_compiler.ml
config for the first time may take a while
rescript: no work to do.
scripts/ninja.js build  11.26s user 4.76s system 258% cpu 6.206 total

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the differences are absorbed by measurement noise, so nothing of note there.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I think it can be merged unless there are other concerns.

CONTRIBUTING.md Outdated

The entry point of the JSOO bundle is located in `jscomp/main/jsoo_main.ml` and the script for running JSOO can be found in `scripts/repl.js`. A full clean build can be done like this:
The entry point of the JSOO bundle is located in `jscomp/main/jsoo_refmt_main.ml`, the code for packing the compiler into a single compiler file is located in `jscomp/snapshot.ninja`, and the script for running JSOO can be found in `scripts/repl.js`. A full clean build can be done like this:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like json_refmt_main.ml has been deleted in this diff. Is some of this file stale?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes I renamed that file bc refmt is gone. Will update.

@cristianoc cristianoc merged commit 9ed51d7 into master Jun 1, 2022
@cristianoc cristianoc deleted the ryyppy/playground-2022 branch June 1, 2022 09:39
mununki pushed a commit to mununki/rescript-compiler that referenced this pull request Jul 15, 2022
* Add all relevant changes from previous 2020 bundle API

For reference on the previous work in 2020, refer to rescript-lang#4518

* Add ninja rule for building jsoo_refmt_main

* Fix formatting edge-cases when formatting ReScript -> ReScript

Configure the parser to pass forPrinter:true when parsing / printing ReScript.
This fixes issues where e.g. Some((1, 2)) get misprinted to Some(1,2) etc.

Relevant bug reports:
- rescript-lang/syntax#274
- rescript-lang/syntax#265

* Introduce initial type hints in playground bundle results

* Adapt playground to newest internal syntax apis

* Make jsoo_refmt_main work again by removing refmt

* Rename jsoo_refmt_main to jsoo_playground_main

* Move jsoo_playground_main to jscomp/main

* Remove irrelevant code

* Remove Refmt_api

* Make repl.js more reliable with missing .mli files

* repl.js: Log target compiler file, set js_playground_main as default

* Update CONTRIBUTING to align with the renamed playground jsoo entrypoint

Co-authored-by: Patrick Stapfer <ryyppy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants