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

Improve performance and clean up the preprocessor #353

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

shym
Copy link
Contributor

@shym shym commented Oct 3, 2023

This PR reworks the preprocessor by:

  • cleaning up with a more complete and uniform reindentation,
  • improving a lot its performance by generating the output into a formatter rather than generating lots of temporary strings.

In a bit more details, the performance gains are obtained by changing the interface of the preprocessor so that it outputs its result into a formatter rather than a string and propagating the change all the way down, so that:

  • the whole output is generated in a single buffer when the target is a string, or directly on stdout in the case of gospel pps,
  • print becomes tail recursive.

Using the example mentioned in #336 (test run in CI, so the performance may vary depending on load...), we go from:

16
allocated_words: 440218559
real	0m1.814s

32
allocated_words: 1749282355
real	0m10.295s

48
allocated_words: 3768016163
real	0m16.260s

to:

16
allocated_words: 2152756
real	0m0.019s

32
allocated_words: 4253492
real	0m0.031s

48
allocated_words: 6354228
real	0m0.044s

@shym
Copy link
Contributor Author

shym commented Oct 4, 2023

Testing on larger inputs seem to show that the preprocessor is handling about 14MB/s. (For comparison, the largest mli in the compiler code base is 76KB). So this is probably good enough for now to say:
Closes #336.

@shym
Copy link
Contributor Author

shym commented Oct 4, 2023

Closes #342

@shym shym linked an issue Oct 4, 2023 that may be closed by this pull request
Adopt a systematic and uniform format for the code of the preprocessor
Document that given style
Add a dune rule so that `dune fmt` reformats the preprocessor
accordingly (when `ex` is available)
Use the name `print_directive`, to be consistent with the rest of the
printing functions
OCaml needs directives to recover the real positions of any preprocessed
source, so preprocessors must output a directive at the very beginning
of their outputs
Change the interface of the preprocessor so that it outputs its result
into a formatter rather than a string
Propagate the change all the way down, so that:
- the whole output is generated in a single buffer when the target is a
  `string`, or directly on `stdout` in the case of `gospel pps`,
- `print` becomes tail recursive.
Ensure that `collapse_spaces` is tail recursive so that it does not
overflow the stack when running on large inputs
That requires an extra list reversal, unfortunately
@shym
Copy link
Contributor Author

shym commented Oct 4, 2023

Updated so that all print_X functions (not print itself, though, which is using) take ppf as their last argument. And rebased.

Copy link
Contributor

@n-osborne n-osborne left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@n-osborne n-osborne merged commit 5bf87c9 into ocaml-gospel:main Oct 5, 2023
2 of 3 checks passed
@shym shym deleted the pps-reworks branch October 5, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants