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

Disable bytecode compilers in ocamltest #504

Merged
merged 1 commit into from Feb 9, 2022

Conversation

stedolan
Copy link
Contributor

@stedolan stedolan commented Feb 2, 2022

Current, ocamltest compiles every test case four times, using all of ocamlc, ocamlc.opt, ocamlopt and ocamlopt.opt. The only reason to compile with both ocamlc and ocamlc.opt is to compare their output, and likewise for ocamlopt.

However, since #128 , we don't actually compare the output of ocamlc and ocamlc.opt, and I'm not sure there much need to compare ocamlopt and ocamlopt.opt either. So, this PR stops ocamltest from using ocamlc and ocamlopt at all, greatly speeding the upstream testsuite. We still compile every case in bytecode and native modes, but only once.

@mshinwell
Copy link
Collaborator

@dra27 does this look right to you?

@stedolan
Copy link
Contributor Author

stedolan commented Feb 2, 2022

CI times:

before after
closure 14m 44s 12m 32s
closure_cfg_local 15m 7s 11m 45s
flambda1 15m 52s 14m 47s
flambda1_cfg_local 16m 22s 16m 43s
flambda2 19m 27s 14m 43s
flambda2_cfg 19m 10s 12m 14s
macos 33m 10s 27m 14s

(no idea what's going on in flambda1_cfg_local beyond "CI times are noisy")

@mshinwell
Copy link
Collaborator

It's a bit of a shame the macos one takes so long, but it is useful to check this. I wonder if we could speed that up somehow. @xclerc do you remember from previous macos problems whether they were usually on the testsuite, or on the build? One option might not be to run the testsuite on that platform until we can get a faster (arm64/M1?) worker.

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

What prompted the pull request was a build issue (#411), but
I can imagine catching problems with the testsuite.

I am wondering: (i) is merge blocked until all checks pass? and
(ii) are checks cancelled when a pull request is merged?
If "no" is the answer to both questions, our policy may be
to merge when the macOS check is still in progress and
the other ones are green. We would not wait, and if anything
breaks we would quite easily find the first breakage and work
on a smaller diff.

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

Another obvious possibility to to only have a daily run of
the check on macOS.

@mshinwell
Copy link
Collaborator

@xclerc maybe we should run the testsuite on macos only for the "push" events, leaving the "pull_request" events to check only the build on macos? I'd still like to make sure PRs don't break the build.

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

I am not sure I follow, don't you want to build on every "push"
if you want to be sure the tip is always buildable on macOS?

Looking at the definitions of the jobs, I wonder whether we could
save some time when building the compiler for dune. By building
the strict minimum there -- i.e. no documentation, no debugger, etc.

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

Looking at the definitions of the jobs, I wonder whether we could save some time when building the compiler for dune. By building the strict minimum there -- i.e. no documentation, no debugger, etc.

For the avoidance of doubt, that would benefit to all jobs, not only the macOS one.

@mshinwell
Copy link
Collaborator

There may well be some simplifications there, indeed.

For macos, just build on pull request pushes, but for pushes to the actual branch, build and then run the testsuite.

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

This might be what we are looking for: https://github.community/t/trigger-workflow-only-on-pull-request-merge/17359/2
i.e. run the job on merge.

@mshinwell
Copy link
Collaborator

Yeah, that's what I was thinking of, but just the build step should also happen as it does at the moment (i.e. every push to a pull request).

@xclerc
Copy link
Contributor

xclerc commented Feb 2, 2022

Great, I will try to set up this configuration tomorrow.

@xclerc xclerc mentioned this pull request Feb 3, 2022
Copy link
Collaborator

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

LGTM! For vague reference, I did something similar - but Windows-specific - in ocaml-multicore/ocaml-multicore@b84109e (which intentionally wasn't merged).

@mshinwell
Copy link
Collaborator

@dra27 thanks

@mshinwell mshinwell merged commit 84ccbef into ocaml-flambda:main Feb 9, 2022
stedolan added a commit to stedolan/flambda-backend that referenced this pull request Mar 7, 2022
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (ocaml-flambda#466)
14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (ocaml-flambda#569)
c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (ocaml-flambda#560)
e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (ocaml-flambda#570)
dab7209a12 flambda-backend: Add Target_system to ocaml/utils (ocaml-flambda#542)
82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (ocaml-flambda#544)
216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (ocaml-flambda#536)
4b56e07c1d flambda-backend: Test naked pointer root handling (ocaml-flambda#550)
40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (ocaml-flambda#537)
f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (ocaml-flambda#365)
ac496bf52e flambda-backend: Disable the local keyword in typing (ocaml-flambda#540)
7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (ocaml-flambda#539)
61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (ocaml-flambda#541)
323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (ocaml-flambda#534)
d8956b09e4 flambda-backend: Persistent environment and reproducibility (ocaml-flambda#533)
4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (ocaml-flambda#506)
7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (ocaml-flambda#376)
6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (ocaml-flambda#295)
96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (ocaml-flambda#530)
42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (ocaml-flambda#504)
58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (ocaml-flambda#471)
10105394de flambda-backend: Use C++ name mangling convention (ocaml-flambda#483)
81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (ocaml-flambda#525)
f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (ocaml-flambda#505)
c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (ocaml-flambda#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
stedolan added a commit that referenced this pull request Mar 7, 2022
64235a382a flambda-backend: Change Float.nan from sNaN to qNaN (#466)
14a8e27063 flambda-backend: Track GC work for all managed bigarray allocations (upstream 11022) (#569)
c3cda96154 flambda-backend: Add two new methods to targetint for dwarf (#560)
e6f1fed2f5 flambda-backend: Handle arithmetic overflow in select_addr (#570)
dab7209a12 flambda-backend: Add Target_system to ocaml/utils (#542)
82d5044871 flambda-backend: Enhance numbers.ml with more primitive types (#544)
216be99334 flambda-backend: Fix flambda_o3 and flambda_oclassic attributes (#536)
4b56e07c1d flambda-backend: Test naked pointer root handling (#550)
40d69cef86 flambda-backend: Stop local function optimisation from moving code into function bodies; opaque_identity fixes for class compilation (#537)
f08ae5851c flambda-backend: Implemented inlining history and use it inside inlining reports (#365)
ac496bf52e flambda-backend: Disable the local keyword in typing (#540)
7d46712f7a flambda-backend: Bugfix for Typedtree generation of arrow types (#539)
61a7b47773 flambda-backend: Insert missing page table check in roots_nat.c (#541)
323bd36d98 flambda-backend: Compiler error when -disable-all-extensions and -extension are used (#534)
d8956b09e4 flambda-backend: Persistent environment and reproducibility (#533)
4a0c89f117 flambda-backend: Revert "Revert bswap PRs (480 and 482)" (#506)
7803705828 flambda-backend: Cause a C warning when CAMLreturn is missing in C stubs. (#376)
6199db5b26 flambda-backend: Improve unboxing during cmm for Flambda (#295)
96b9e1ba6d flambda-backend: Print diagnostics at runtime for Invalid (#530)
42ab88e8a1 flambda-backend: Disable bytecode compilers in ocamltest (#504)
58c72d5476 flambda-backend: Backport ocaml/ocaml#10595 from upstream/trunk (#471)
10105394de flambda-backend: Use C++ name mangling convention (#483)
81881bbf88 flambda-backend: Local allocation test no longer relies on lifting (#525)
f5c47190f6 flambda-backend: Fix an assertion in Closure that breaks probes (#505)
c2cf2b2a14 flambda-backend: Add some missing command line arguments to ocamlnat (#499)

git-subtree-dir: ocaml
git-subtree-split: 64235a382a0424cced40eed328ddf1dfb9645f87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants