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
Use compact regions? #4457
Comments
Compact regions do not support functions, and compaction is "very costly" if you want to retain sharing, so perhaps this is not a good idea. |
But we do not serialise functions… I ran a quick test. The key change is restricted to one line in
I ran Perhaps this makes sense after all. Compact regions seem to be available from GHC 8.0, but I'm not sure how well they worked in that version of GHC. I've tested using 8.6.5. |
My plan is to push my changes, restricted to GHC 8.6 and higher. The documentation claims that the feature "is supported by GHC 8.2 and later", but I don't want to spend time benchmarking older versions of GHC. I'm depending on the lower-level library ghc-compact, rather than compact, because the latter does not support GHC 8.8 (I'm guessing that it is not maintained). Any objections? It might also be a good idea to always use the serialised interfaces (#4316). |
Since “compacting data involves copying it”, it probably it makes sense to add a verbosity flag keeping track of individual and cumulated And in view of
, an entry in the Agda-internal time profiling list separate from deserialisation probably also makes sense. |
I've pushed my changes. Feel free to implement the things that you suggested. |
Given that we are (hopefully?) about to release 2.6.1 I'll add a flag for this feature and turn it off by default. For that reason I'll also add some documentation. |
I'll add that. |
Profiling information for the test case above:
These numbers look suspicious to me. I'll test a bit more. |
I got the following numbers for std-lib-test:
The second option does not look very good. The last option is (for these particular test runs) slightly slower than the first option, but uses much less memory. I'll activate decoding of serialised interfaces, as suggested before (#4316), when compaction is available and in use. |
By default compact regions are not used. The compact region code is now enabled starting from GHC 8.2.
Nice work! About the option name: My first association for "compact regions" was to use compact source code regions (in the sense of I suggest to have a prefix to the name that distinguishes this option as something just tuning the performance of Agda, as opposed to changing type theory, changing interaction, or changing the produced executable. My color of the bike shed: |
My plan was to remove this option once we decide whether this is a good idea or not. However, if we come to the conclusion that this is something that should be tunable by the user, then a better name might make sense. If we anticipate having multiple performance tuning options, then I would prefer something like |
As far as I can tell most of the CI tests are only run for a single version of GHC. I'd like to make sure that |
Given the performance improvements I've seen I suggest that we turn on |
From |
Can I run the Travis tests without creating a pull request or pushing a commit? I don't want to create a pull request for something that I don't want human feedback on. I get notifications for all pull requests, and in quite a few cases the only purpose of a pull request is to run the test suite. These pull requests also, in my opinion, pollute the GitHub history. |
I believe Travis runs on all branches, regardless of PRs, so you can just push a temporary branch with a |
From
So, you should name your branch |
Ah, blind luck that this is what I do already... |
All that Travis magic was written by @L-TChen. |
Thanks, that seems to work. |
Unfortunately Agda failed to build using GHC 8.0.2 and 8.2.2, seemingly due to lack of time or space. Perhaps I can run the tests locally instead. |
No, for GHC 8.2.2 the problem was that Agda failed to create the interface file for |
Now compact regions are always active (for GHC 8.4 and later). The option has been removed, I see no reason for not using compact regions. As an aside, it seems as if memory consumption (with or without compact regions) is substantially lower with GHC 8.6.5 than with 8.4.4, and perhaps even lower with 8.8.2. |
The test suite is failing on my PC with GHC 8.0.2 because of this change. |
Could we roll back this change until those issues have been resolved? We should avoid as much as possible to be in a situation where the test suite does not run successfully. |
I'll disable the broken test cases. |
By the way, note that the problems you see are not caused by the use of compact regions, but by the fix of #4316 (which is only active if compact regions are in use). |
Perhaps we could use "compact regions" to avoid garbage-collecting data read from interface files.
The text was updated successfully, but these errors were encountered: