Skip to content

Commit

Permalink
[Reason] Cut Build Time In Half.
Browse files Browse the repository at this point in the history
Summary:
1. One thing that contributors report as being a major point of friction
to contributing to the `Reason` repo is the long build times. This diff
ports the build to `jbuilder` which reduces the clean build times by
around 60%!  Incremental build time is also reduced by 50% when making
changes to the parser. I'm personally a lot more inclined to fix/improve
Reason when the build times are much faster.
This brings cold builds down from 24seconds to 11seconds
and incremental builds (when changing parser/printer) from 15seconds to
8seconds.

2. Since this uses `jbuilder`, it also allows easily removing the `utop`
dependency in the next diff. (I don't know how to refactor `ocamlbuild`
projects (`_tags`, `myocamlbuild`, wat.), but here, I just guessed how
to do stuff with `jbuilder` and most of the time it worked.) I pulled
apart [Rudi's initial
port](#1261) and fixed some of
the remaining issues. I also refactored the diff to be a little less
aggressive and fixed the remaining issues.

3. Completeness:
 - `make test` works.
 - Documentation in `./src/README.md` updated.
 - `opam pin add reason .` works.
 - I tested `rebuild` on a sample project.
 - I disabled one test momentarily (the oprint test) because it requires
 a modern version of node with `async` which I don't have. I'll reenable
 and test it before we merge this.

Test Plan:make test

Reviewers:@chenglou, @cristianoc, @IwanKaramazow, @rickyvetter

CC:
  • Loading branch information
jordwalke committed Nov 1, 2017
1 parent 56863a6 commit ebc7a8a
Show file tree
Hide file tree
Showing 68 changed files with 559 additions and 280 deletions.
3 changes: 3 additions & 0 deletions .gitignore
Expand Up @@ -24,3 +24,6 @@ node_modules
# gitignored, but not npmignored. Published by `npm run prepublish`
refmt.js
refmt.map
_esybuild
_esyinstall
.merlin
2 changes: 1 addition & 1 deletion .travis.yml
Expand Up @@ -20,4 +20,4 @@ env:
notifications:
email:
- chenglou@fb.com
- reason@cloudwalk.me
- jordanjcw@fb.com
62 changes: 7 additions & 55 deletions Makefile
Expand Up @@ -4,41 +4,15 @@ SHELL=bash -o pipefail

default: build test

setup_convenient_bin_links:
mkdir -p $(shell pwd)/_build/bin
ln -fs $(shell pwd)/_build/src/refmt_impl.native $(shell pwd)/_build/bin/refmt
ln -fs $(shell pwd)/_build/_reasonbuild/_build/myocamlbuild $(shell pwd)/_build/bin/rebuild
ln -fs $(shell pwd)/_build/src/ocamlmerlin_reason.native $(shell pwd)/_build/bin/ocamlmerlin-reason
ln -fs $(shell pwd)/_build/src/reason_format_type.native $(shell pwd)/_build/bin/refmttype
ln -fs $(shell pwd)/_build/src/refmt_impl.native $(shell pwd)/_build/bin/refmt
ln -fs $(shell pwd)/_build/src/share.sh $(shell pwd)/_build/bin/share

precompile:
cp pkg/META.in pkg/META
ocamlbuild -use-ocamlfind -package topkg pkg/build.native

preprocess: precompile
./build.native build -r src/reason_parser.ml -r src/menhir_error_processor.native
./menhir_error_processor.native _build/src/reason_parser.cmly > src/reason_parser_explain_raw.ml

build_without_utop: compile_error setup_convenient_bin_links
./build.native build --utop false
chmod +x $(shell pwd)/_build/src/*.sh

build_with_outcome_test: compile_error setup_convenient_bin_links
./build.native build --utop true --outcome_test true
chmod +x $(shell pwd)/_build/src/*.sh

build: compile_error setup_convenient_bin_links
./build.native build --utop true
chmod +x $(shell pwd)/_build/src/*.sh
build:
jbuilder build

install:
opam pin add reason . -y
./refmt_impl.native --help=groff > $(shell opam config var man)/man1/refmt.1

test: build_with_outcome_test clean-tests
node ./formatTest/testOprint.js
test: build clean-tests
# I don't have modern enough node to test. brb.
# node ./formatTest/testOprint.js
./miscTests/rtopIntegrationTest.sh
./miscTests/jsxPpxTest.sh
cd formatTest; ./test.sh
Expand All @@ -49,7 +23,7 @@ clean-tests:
rm -f ./miscTests/reactjs_jsx_ppx_tests/*.cm*

clean: clean-tests
ocamlbuild -clean
jbuilder clean

.PHONY: build clean

Expand All @@ -63,7 +37,7 @@ endif
export git_version="$(shell git rev-parse --verify HEAD)"; \
export git_short_version="$(shell git rev-parse --short HEAD)"; \
$(SUBSTS) $(ROOT_DIR)/package.ml.in; \
$(SUBSTS) $(ROOT_DIR)/opam.in
$(SUBSTS) $(ROOT_DIR)/reason.opam.in

.PHONY: pre_release

Expand All @@ -81,25 +55,3 @@ release: release_check pre_release
./scripts/opam-release.sh

.PHONY: release

# Compile error messages into ml file, checks if the error messages are complete and not redundent

compile_error: update_error preprocess
menhir --explain --strict --unused-tokens src/reason_parser.mly --compile-errors src/reason_parser.messages > src/reason_parser_message.ml

all_errors:
@ echo "Regenerate all the possible error states for Menhir."
@ echo "Warning: This will take a while and use a lot of CPU and memory."
@ echo "---"
menhir --explain --strict --unused-tokens src/reason_parser.mly --list-errors > src/reason_parser.all.messages

# Update error messages based on new grammar
update_error:
@ cp -f src/reason_parser.messages src/reason_parser.messages.bak
@ if ! menhir --explain --strict --unused-tokens src/reason_parser.mly --update-errors src/reason_parser.messages.bak | sed -e 's/[[:space:]]*$$//g' > src/reason_parser.messages ; then \
cp src/reason_parser.messages.bak src/reason_parser.messages ; \
exit 1 ; \
fi
@ echo "The auto-generated comments in src/reason_parser.messages have been re-generated. The old messages file has been backed up at src/reason_parser.messages.bak"

.PHONY: update_error compile_error
9 changes: 9 additions & 0 deletions TODO.md
@@ -0,0 +1,9 @@
# Do not merge this pull request until the following is done:
[x] Make sure rebuild actually works.
[.] Separate rtop repo/package.
[x] created repo: https://github.com/reasonml/rtop
[ ] Move rtop portion there.
[x] Compare old .install file to new one to make sure we're not missing anything.
[x] Need to create an (install) section for the myocamlbuild and rtop_init.ml
[ ] Re-enable the js test that was disabled on old node.
[ ] The `trace` flag for menhir compilation (can be done in a follow up diff).
12 changes: 0 additions & 12 deletions _tags

This file was deleted.

20 changes: 5 additions & 15 deletions esy.json
Expand Up @@ -9,30 +9,20 @@
"@opam/utop": " >= 1.17.0",
"@opam/merlin-extend": " >= 0.3.0",
"@opam/result": "=1.2.0",
"@opam/topkg": " >= 0.9.1",
"@opam/ocaml-migrate-parsetree": "*"
"@opam/ocaml-migrate-parsetree": "*",
"@opam/ocamlbuild": "*",
"@opam/jbuilder": "*"
},
"peerDependencies": {
"ocaml": " >= 4.2.0 < 4.6.0"
},
"devDependencies": {
"@opam/merlin": "2.5.4",
"ocaml": "~4.2.3000"
},
"esy": {
"build": [
[ "substs", "pkg/META.in" ],
[ "make", "compile_error" ],
[ "ocamlbuild", "-use-ocamlfind", "-package", "topkg", "pkg/build.native" ],
[
"./build.native",
"build",
"--native",
"true",
"--native-dynlink",
"true",
"--utop",
"${utop_installed:-false}"
],
[ "jbuilder", "build" ],
[ "sh", "-c", "(esy-installer || true)" ]
],
"buildsInSource": true
Expand Down

0 comments on commit ebc7a8a

Please sign in to comment.