-
Notifications
You must be signed in to change notification settings - Fork 24
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
Evacuate set and map iterators during garbage collection #441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dwightguth
changed the title
Evacuate iter
Evacuate set and map iterators during garbage collection
Oct 18, 2021
Baltoli
approved these changes
Oct 19, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor points that may not actually need addressing, but otherwise LGTM.
theo25
approved these changes
Oct 19, 2021
31 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing the new_gc branch against the K frontend test suite, we uncovered a problem. If a set or map iterator (resulting from a set or map choice operation during pattern matching) is live during garbage collection, the pointers inside that iterator were not being fixed up to point to the correct semispace, even though the set or map that the iterator was associated with was being relocated by the GC. This led to cases where the forwarding pointer was being treated as if it were a pointer to actual data, which of course caused memory corruption to occur.
The solution is a bit tricky. Essentially, we introduce a new SortCategory MapIterator and SetIterator and split the %iter type in llvm IR into %mapiter and %setiter. This allows our LLVM pass to correctly identify that certain live pointers are pointers to iterators. (We also fix up the address space of those live pointers so that they are treated as garbage-collected pointers by the RewriteStatepointForGC pass.)
We also have to introduce a new change upstream to immer in order to store the information inside the
champ_iterator
type that is needed in order to correctly compute the base pointers and derived offsets necessary to evacuate the iterator. The upstream PR is here: arximboldi/immer#190. You shouldn't need to review it; the maintainers of the library will. Feel free to refer to it for reference though if you want to understand what is happening in our PR.Once all these changes are introduced, we simply have to modify the collection routine to correctly evacuate the pointers inside these objects. We do this by recording them as needing evacuation when we iterate through the list of roots, and then performing a last stage of garbage collection after evacuating the old generation, where we evacuate the pointers that we stored earlier. This does rely on a certain internal knowledge of the storage layout of iterators in the immer library, but since we are pinned to a specific commit, this is fine. It will just need to be potentially updated if we bump the submodule version, but we don't do that terribly frequently.
Also included in this change are some quick changes to the search function's top-level logic. Basically, instead of storing the results of a single rewrite step on the heap, we store them in a global variable and then use the existing
migrateRoots
functionality to handle fixing up the pointers to the search results when garbage collection happens. This is necessary because it is possible for a step to compute several search results, trigger garbage collection, and then continue with further pattern matching, in which case the garbage collection algorithm has to be able to fix up the pointers to search results that were already computed prior to collection. This was simply the easiest way of accomplishing this.