Make full_thread_state a shared pointer#3966
Open
abellina wants to merge 7 commits intoNVIDIA:mainfrom
Open
Conversation
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
Collaborator
Author
|
build |
Contributor
Greptile Summary
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Thread as "Thread"
participant Adaptor as "spark_resource_adaptor"
participant StateMap as "threads map"
participant SharedPtr as "shared_ptr<full_thread_state>"
Thread->>Adaptor: "associate_thread_with_task(thread_id, task_id)"
Adaptor->>StateMap: "threads.emplace(thread_id, ...)"
StateMap->>SharedPtr: "make_shared<full_thread_state>(...)"
SharedPtr-->>StateMap: "shared_ptr created"
StateMap-->>Adaptor: "emplace successful"
Adaptor->>SharedPtr: "thread->second->state"
SharedPtr-->>Adaptor: "access thread state via ->"
Adaptor-->>Thread: "association complete"
Thread->>Adaptor: "do_allocate(bytes, blocking=true)"
Adaptor->>StateMap: "threads.find(thread_id)"
StateMap-->>Adaptor: "iterator to thread entry"
Adaptor->>SharedPtr: "thread->second->state = THREAD_ALLOC"
alt Thread needs to block
Adaptor->>SharedPtr: "thread->second->before_block()"
loop Wait until unblocked
Adaptor->>SharedPtr: "thread->second->wake_condition->wait(lock)"
Adaptor->>StateMap: "thread = threads.find(thread_id)"
alt Thread was removed
StateMap-->>Adaptor: "thread == threads.end()"
Adaptor->>Adaptor: "break loop"
else Thread still exists
Adaptor->>SharedPtr: "check is_blocked(thread->second->state)"
end
end
alt Thread still exists after wait
Adaptor->>SharedPtr: "thread->second->after_block()"
end
end
Adaptor->>SharedPtr: "transition(thread->second, THREAD_RUNNING)"
SharedPtr-->>Adaptor: "state updated"
Adaptor-->>Thread: "allocation complete"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
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
Collaborator
Author
|
build |
Contributor
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
Collaborator
Author
|
build |
Contributor
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
reviewed
Nov 18, 2025
Collaborator
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?
Collaborator
Author
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. |
Collaborator
Author
|
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. |
Collaborator
Author
|
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 |
Contributor
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
Collaborator
Author
|
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 |
revans2
approved these changes
Dec 22, 2025
ttnghia
reviewed
Dec 23, 2025
| * 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) |
Collaborator
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.
Collaborator
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.
ttnghia
reviewed
Dec 23, 2025
| * 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) |
ttnghia
reviewed
Dec 23, 2025
| } | ||
|
|
||
| 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) |
Collaborator
|
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. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
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.