Optimise "include struct ... end" in more cases by stedolan · Pull Request #11134 · ocaml/ocaml

stedolan · 2022-03-21T15:19:35Z

The patch in #832 introduced a more efficient compilation scheme for the pattern

include (struct ... end : sig ... end)

which avoids creating many local variables and an intermediate module block, speeding compilation time (and runtime too, not that it matters much for initialisation).

However, that patch omits two cases:

When the coercion is trivial (i.e. the sig ... end mentions all runtime components of the module)
When there is no coercion at all (i.e. include struct ... end)

Such cases were considered when #832 was written:

The trick is currently not applied to include (struct .... end); it would not be difficult to do so, but there is no real reason to use that form anyway (except perhaps to simplify code generators, or to customize warnings locally in a section),

This exact use case has indeed come up: various ppx generators use the include struct ... end pattern to customize warnings locally in a section.

This patch extends the existing optimisation to cover those cases as well.

lpw25

LGTM

lthls · 2022-03-21T16:30:21Z

Have you checked that the test passes with Flambda ? I did a quick experiment with a released compiler (4.13), and the test would fail, but not because of the module allocations. Instead, the allocs record is allocated after the first call to Gc.minor_words, so the first call to count sees this allocation.

stedolan · 2022-03-22T09:38:30Z

Have you checked that the test passes with Flambda ? I did a quick experiment with a released compiler (4.13), and the test would fail, but not because of the module allocations. Instead, the allocs record is allocated after the first call to Gc.minor_words, so the first call to count sees this allocation.

Thanks, I hadn't realised it wasn't currently checked by CI! Fixed now.

gasche · 2022-03-22T11:38:12Z

(Why isn't the CI checking Flambda, didn't @lthls fix all remaining Flambda issues?)

lthls · 2022-03-22T11:55:04Z

The github CI setup has changed completely with the merge of multicore, and I haven't taken the time to investigate how to add a flambda build to it yet.
I think that one of the Inria CI jobs builds and tests with flambda enabled though, so if there is a Flambda-specific failure we should see it there.

abbysmal · 2022-03-22T12:28:34Z

@lthls I am currently working on revamping the post-Multicore Github Action code, are you suggesting adding back a job similar to flambda-full as in the 4.14 branch?
I can add that to my list.

lthls · 2022-03-22T12:53:05Z

@Engil My preference would be for configuring one of the other jobs to use flambda. Previously we had a single job checking the no-naked-pointers mode and flambda, for example.
But if it's simpler for you to just add a new job, that's still a good start.

abbysmal · 2022-03-22T13:00:58Z

@lthls I will look into the options, thank you for your answer.

xavierleroy · 2022-03-22T17:40:36Z

I think that one of the Inria CI jobs builds and tests with flambda enabled though

The "main" and "precheck" jobs in the Jenkins CI at Inria tests flambda on 5 different configurations, and the "other-configs" job tests it too. So, flambda is tested well, just not within Github Actions.

ce88833 Merge flambda-backend changes b7506bb Revert "Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12)" 183f688 Add config option to enable/disable stack allocation (#22) ee7c849 If both the type and mode of an ident are wrong, complain about the type. (#19) 44bade0 Allow submoding during module inclusion checks (#21) de3bec9 Add subtyping between arrows of related modes (#20) 93d8615 Enable the local keywords even when the local extension is off (#18) 81dd85e Documentation for local allocations b05519f Fix a GC bug in local stack scanning (#17) 9f879de Fix __FUNCTION__ (#15) a78975e Optimise "include struct ... end" in more cases (ocaml/ocaml#11134) b819c66 Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12) bb363d4 Optimise the allocation of optional arguments (#11) git-subtree-dir: ocaml git-subtree-split: ce88833

ce88833 Merge flambda-backend changes b7506bb Revert "Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (oxcaml#12)" 183f688 Add config option to enable/disable stack allocation (oxcaml#22) ee7c849 If both the type and mode of an ident are wrong, complain about the type. (oxcaml#19) 44bade0 Allow submoding during module inclusion checks (oxcaml#21) de3bec9 Add subtyping between arrows of related modes (oxcaml#20) 93d8615 Enable the local keywords even when the local extension is off (oxcaml#18) 81dd85e Documentation for local allocations b05519f Fix a GC bug in local stack scanning (oxcaml#17) 9f879de Fix __FUNCTION__ (oxcaml#15) a78975e Optimise "include struct ... end" in more cases (ocaml/ocaml#11134) b819c66 Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (oxcaml#12) bb363d4 Optimise the allocation of optional arguments (oxcaml#11) git-subtree-dir: ocaml git-subtree-split: ce88833

stedolan · 2022-05-23T12:34:28Z

This patch had a bug because of a missing case in all_idents, fixed in the most recent push. (The testsuite didn't catch it because the bug requires a nested module. The version of the test in the new commit does catch it)

454150b flambda-backend: Speed up testsuite (#658) 8362f9e flambda-backend: Speed up builds (#585) a527cab flambda-backend: Update backends for changes from ocaml-jst ce88833 Merge flambda-backend changes b7506bb Revert "Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12)" 183f688 Add config option to enable/disable stack allocation (#22) ee7c849 If both the type and mode of an ident are wrong, complain about the type. (#19) 44bade0 Allow submoding during module inclusion checks (#21) de3bec9 Add subtyping between arrows of related modes (#20) fe8a98b flambda-backend: Save Mach as Cfg after Selection (#624) 2b205d8 flambda-backend: Clean up algorithms (#611) 93d8615 Enable the local keywords even when the local extension is off (#18) 524f0b4 flambda-backend: Initial refactoring of To_cmm (#619) 81dd85e Documentation for local allocations b05519f Fix a GC bug in local stack scanning (#17) 9f879de Fix __FUNCTION__ (#15) 0bf75de flambda-backend: Refactor and correct the "is pure" and "can raise" (port upstream PR#10354 and PR#10387) (#555) d234bfd flambda-backend: Cpp mangling is now a configuration option (#614) 20fc614 flambda-backend: Check that stack frames are not too large (#10085) (#561) 5fc2e95 flambda-backend: Allow CSE of immutable loads across stores (port upstream PR#9562) (#562) 2a650de flambda-backend: Backport commit fc95347 from trunk (#584) a78975e Optimise "include struct ... end" in more cases (ocaml/ocaml#11134) b819c66 Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12) bb363d4 Optimise the allocation of optional arguments (#11) 31651b8 flambda-backend: Improved ARM64 code generation (port upstream PR#9937) (#556) f0b6d68 flambda-backend: Simplify processing and remove dead code (error paths) in asmlink (port upstream PR#9943) (#557) 90c6746 flambda-backend: Improve code-generation for inlined comparisons (port upstream PR#10228) (#563) git-subtree-dir: ocaml git-subtree-split: 454150b

nojb · 2022-12-31T09:08:38Z

This PR looks ready to merge for all intents and purposes. @stedolan: can you rebase the patch? Thanks!

nojb · 2023-01-13T08:15:00Z

I pushed a commit to fix a check-typo failure. Looks good to merge once CI passes. Thanks!

gasche · 2023-01-13T08:52:33Z

The "normal build" is failing with "Unable to connect to azure.archive.ubuntu.com", which is obviously unrelated to the current PR. But it seems reproducible: I asked to "re-run failed jobs" and I believe that it failed again. cc @dra27 our Github Actions overlord.

nojb · 2023-01-13T09:07:24Z

Thanks, merged (the CI was green except for the check-typo fix before).

gasche · 2023-01-13T09:07:49Z

Note: this unable-to-connect CI failure also happens on trunk, so everyone merging something will get emails about it.

dra27 · 2023-01-13T09:23:35Z

There seems to be something up with GitHub Actions this morning! Various issues appearing on the runner-images issue tracker.

lpw25 approved these changes Mar 21, 2022

View reviewed changes

stedolan added a commit to ocaml-flambda/ocaml-jst that referenced this pull request Mar 21, 2022

Optimise "include struct ... end" in more cases (ocaml/ocaml#11134)

41db038

stedolan force-pushed the transl-include-struct branch 2 times, most recently from 41db038 to 1b71da5 Compare March 22, 2022 09:35

stedolan force-pushed the transl-include-struct branch from 1b71da5 to 25a7eeb Compare March 22, 2022 09:39

stedolan added a commit to ocaml-flambda/ocaml-jst that referenced this pull request Mar 22, 2022

Optimise "include struct ... end" in more cases (ocaml/ocaml#11134)

a78975e

stedolan mentioned this pull request May 23, 2022

Bugfix for Translmod.all_idents oxcaml/oxcaml#659

Merged

stedolan added 2 commits January 12, 2023 14:12

Optimise "include struct ... end" in more cases

74c2945

Bugfix for Translmod.all_idents

684b8f5

stedolan force-pushed the transl-include-struct branch from 5117af4 to 684b8f5 Compare January 12, 2023 14:12

check-typo

6867da1

nojb added the merge-me label Jan 13, 2023

nojb merged commit c71cea8 into ocaml:trunk Jan 13, 2023

stedolan deleted the transl-include-struct branch February 15, 2023 10:51

Conversation

stedolan commented Mar 21, 2022

Uh oh!

lpw25 left a comment

Choose a reason for hiding this comment

Uh oh!

lthls commented Mar 21, 2022

Uh oh!

stedolan commented Mar 22, 2022

Uh oh!

gasche commented Mar 22, 2022

Uh oh!

lthls commented Mar 22, 2022

Uh oh!

abbysmal commented Mar 22, 2022

Uh oh!

lthls commented Mar 22, 2022

Uh oh!

abbysmal commented Mar 22, 2022

Uh oh!

xavierleroy commented Mar 22, 2022

Uh oh!

stedolan commented May 23, 2022

Uh oh!

nojb commented Dec 31, 2022

Uh oh!

nojb commented Jan 13, 2023

Uh oh!

gasche commented Jan 13, 2023

Uh oh!

nojb commented Jan 13, 2023

Uh oh!

gasche commented Jan 13, 2023

Uh oh!

dra27 commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dra27 commented Jan 13, 2023 •

edited

Loading