Make full_thread_state a shared pointer#3966
Conversation
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
|
build |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 file reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
…ark-rapids-jni into shared_ptr_full_thread_state
|
build |
There was a problem hiding this comment.
1 file reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
|
build |
There was a problem hiding this comment.
2 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
revans2
left a comment
revans2
left a comment
There was a problem hiding this comment.
Have you measured the performance impact of this? shared_ptr has o do a lot more work than a normal pointer. Did you run into problems anywhere that needed this? Are you planning more changes that require it, like looping through a data structure and you can modify it as you loop?
Main change I was looking to do was be able to take the lock to get state, and then return the state object as is without a lock. This implies I can add locks inside of the per-thread state object to protect its internal datastructures. But this is something I am planning for later, as moving state inside of In terms of perf, I tested it with other patches and performance in the plugin under NDS never got worse. But, I can split this up and rework the patch that is coming after this to use raw pointers and then we can just measure adding it when absolutely necessary. |
|
Lets punt on this, I don't have a great answer to the performance question on this change on its own, and it's not strictly necessary to what I am trying to accomplish right now. |
|
never mind, I do need shared pointers. I am storing full_thread_state in different maps in one of my patches. I'll have to reopen, let me test this patch independently |
There was a problem hiding this comment.
2 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
|
I ran NDS at sf3k, and I don't see significant changes. It's slightly faster at the end, but it's all in the noise: Results: benchmark: Previous (242200.0 ms) vs Current (241000.0 ms) Diff 1200 E2E 1.00x |
| * verification. | ||
| */ | ||
| void transition(full_thread_state& state, thread_state const new_state) | ||
| void transition(std::shared_ptr<full_thread_state> state, thread_state const new_state) |
There was a problem hiding this comment.
Nit: We can use std::shared_ptr<> const& as parameter. Doing so will avoid the atomic ops when creating a new shared_ptr and reduce overhead.
There was a problem hiding this comment.
However, if the intent of this change is to allow this function to continue runing while leaving the state at the caller to be destroyed independently then this can be left as-is.
| * Checkpoint all of the metrics for a thread. | ||
| */ | ||
| void checkpoint_metrics(full_thread_state& state) | ||
| void checkpoint_metrics(std::shared_ptr<full_thread_state> state) |
| } | ||
|
|
||
| bool is_thread_bufn_or_above(JNIEnv* env, full_thread_state const& state) | ||
| bool is_thread_bufn_or_above(JNIEnv* env, std::shared_ptr<full_thread_state> state) |
|
NOTE: release/26.02 has been created from main. Please retarget your PR to release/26.02 if it should be included in the release. |
|
NOTE: release/26.04 has been created from main. Please retarget your PR to release/26.04 if it should be included in the release. |
|
NOTE: release/26.06 has been created from main. Please retarget your PR to release/26.06 if it should be included in the release. |
There was a problem hiding this comment.
Pull request overview
This PR updates the spark_resource_adaptor thread-tracking state so that each full_thread_state is heap-allocated and referenced via std::shared_ptr, enabling safe retention of thread state beyond the lifetime of the originating thread.
Changes:
- Replaced
threadsmap value type fromfull_thread_statetostd::shared_ptr<full_thread_state>. - Updated state access patterns throughout the state machine from
.to->. - Updated helper APIs (
transition,checkpoint_metrics,is_thread_bufn_or_above) to accept shared-pointer state.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto const was_threads_inserted = threads.emplace( | ||
| thread_id, full_thread_state(thread_state::THREAD_RUNNING, thread_id, task_id)); | ||
| thread_id, | ||
| std::make_shared<full_thread_state>(thread_state::THREAD_RUNNING, thread_id, task_id)); |
| auto const was_inserted = threads.emplace( | ||
| thread_id, std::make_shared<full_thread_state>(thread_state::THREAD_RUNNING, thread_id)); |
| * verification. | ||
| */ | ||
| void transition(full_thread_state& state, thread_state const new_state) | ||
| void transition(std::shared_ptr<full_thread_state> state, thread_state const new_state) |
| * Checkpoint all of the metrics for a thread. | ||
| */ | ||
| void checkpoint_metrics(full_thread_state& state) | ||
| void checkpoint_metrics(std::shared_ptr<full_thread_state> state) |
| } | ||
|
|
||
| bool is_thread_bufn_or_above(JNIEnv* env, full_thread_state const& state) | ||
| bool is_thread_bufn_or_above(JNIEnv* env, std::shared_ptr<full_thread_state> state) |
Contributes to #3905
This change is simply making
full_thread_statea shared pointer, and updating all calls to use->instead of..We do this so that when we pass
full_thread_statearound, we are incrementing the ref count in the pointer and even if the thread disappears, we are still able to hold a reference to the state object until we exit the function call.This PR doesn't change behavior, but in the future we can do things like: get the thread state under a lock, unlock, return thread state and use that object without fear that it will be destroyed by another thread.