Move heap ownership info from chunk to pagemap #4371
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
Prior to this commit, the chunk kept track of the heap ownership info in the `chunk->actor` field. This worked but was not ideal as noted in the `ponyint_heap_owner` function that this is a case of false sharing where the chunk owner needs the chunk but all other actors only need the chunk owner only. This led to lots of pointer chasing as the pagemap was used to retrieve the chunk pointerand then the chunk had to be loaded to retrieve the owning actor pointer. This commit removes the `chunk->actor` field further slimming down both the `small_chunk` and `large_chunk` so both now fit within 32 bytes (on 64 bit architectures). The chunk ownership information is now kept in the lowest level of the pagemap next to the chunk information. This removes the previous pointer chasing because the chunk owning actor pointer is retrieved at the same time as the chunk pointer without needing to load the chunk itself. This is a fairly standard tradeoff of memory for performance where we're now storing more data in the pagemap to minimize pointer chasing. The expectation is that this will have a net positive impact on performance but no benchmarks have been run to validate this assumption.
SeanTAllen
added
the
changelog - changed
jemc
added
do not merge
ponylang-main
added
the
discuss during sync
SeanTAllen
added
changelog - changed
|
From Sean: We only want to merge this if #4369 doesn't have a straightforward fix we can implement quickly. If that one does have a straightforward fix, we we want to fix that and release quickly. Otherwise, this PR can be merged and be subjected to a while of nightly stress tests before it gets released. |
ponylang-main
removed
the
discuss during sync
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.
This is a reopening of a PR from @dipinhora - PR #4368.
It was merged too soon (I didn't see the "do not merge" label).