-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
Move heap ownership info from chunk to pagemap #4368
Conversation
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.
Hi @dipinhora, The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be:
Thanks. |
return chunk; | ||
} | ||
|
||
chunk_t* ponyint_pagemap_get(const void* addr, pony_actor_t** actor) |
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.
I think this should get a bit of documentation that actor is being set. It took me a bit to figure that out.
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.
added a comment.. let me know if it's not clear enough..
return chunk; | ||
} | ||
|
||
chunk_t* ponyint_pagemap_get(const void* addr, pony_actor_t** actor) |
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.
I think noting why we have a return chunk_t and and out parameter actor rather than two out parameters, would be good documentation for this function.
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.
added a comment.. let me know if it's not clear enough..
pony_assert(ix <= (uintptr_t)level[PAGEMAP_LEVELS - 1].size); | ||
pony_assert(ix/2 <= (uintptr_t)level[PAGEMAP_LEVELS - 1].mask); | ||
next_node = &(((PONY_ATOMIC_RVALUE(void*)*)node)[ix]); | ||
atomic_store_explicit(next_node, actor, memory_order_release); |
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.
note from sync: please document your understanding of the atomics and their safety here as you stated in the sync call.
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.
added a comment.. let me know if it's not clear enough..
release notes added |
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.
With the small change to fix the release notes format (we only do newlines for new paragraphs), I'm good with this. @jemc, can you also give another review with an eye towards comments and "documentation"?
This reverts commit a9343c5.
This reverts commit a1a1446.
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 theponyint_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 thesmall_chunk
andlarge_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.