[MINOR][CORE] Remove redundant synchronized in ThreadUtils#50210
Closed
jinkachy wants to merge 1 commit intoapache:masterfrom
Closed
[MINOR][CORE] Remove redundant synchronized in ThreadUtils#50210jinkachy wants to merge 1 commit intoapache:masterfrom
jinkachy wants to merge 1 commit intoapache:masterfrom
Conversation
beliefer
reviewed
Mar 7, 2025
Contributor
|
Please config github action. |
beliefer
changed the title
srowen
approved these changes
Mar 7, 2025
Member
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
Agreed, looks left over from some other previous version. This is the only synchronized method which won't do anything
Author
done |
Member
|
Merged to master for Apache Spark 4.1.0. Thank you, @jinkachy and all. |
SteNicholas
added a commit
to apache/celeborn
that referenced
this pull request
Mar 13, 2025
…adUtils#sameThreadExecutorService ### What changes were proposed in this pull request? Remove the redundant synchronized keyword from the `isTerminated` method in `sameThreadExecutorService()` implementation within the `ThreadUtils` class. The method was using both a synchronized block and an explicit `ReentrantLock`, which is unnecessary and potentially problematic. Backport apache/spark#50210. ### Why are the changes needed? The changes are needed for several reasons: 1. **Eliminates redundant synchronization**: The current implementation uses both synchronized keyword and explicit ReentrantLock, which is redundant and creates unnecessary overhead. 2. **Reduces potential deadlock risks**: Using two different locking mechanisms (built-in synchronized and explicit `ReentrantLock`) in the same method could lead to complex lock ordering issues and increase deadlock risks. Although the risk of deadlock in the current implementation is low, if someone modifies the code in the future and adds a method that acquires these two locks in a different order, it would introduce a deadlock risk. 3. **Improves performance**: Removing the unnecessary synchronization layer reduces lock contention and context switching overhead. 4. **Code consistency**: Other methods in the same class only use `ReentrantLock` for synchronization, so removing synchronized makes the code style more consistent. 5. **More precise control**: `ReentrantLock` already provides all the synchronization needed with more features than the implicit synchronized keyword. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. Closes #3150 from SteNicholas/CELEBORN-1910. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: SteNicholas <programgeek@163.com>
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.
What changes were proposed in this pull request?
This PR removes the redundant
synchronizedkeyword from theisTerminatedmethod insameThreadExecutorService()implementation within theThreadUtilsclass. The method was using both asynchronizedblock and an explicitReentrantLock, which is unnecessary and potentially problematic.Why are the changes needed?
The changes are needed for several reasons:
Eliminates redundant synchronization: The current implementation uses both
synchronizedkeyword and explicitReentrantLock, which is redundant and creates unnecessary overhead.Reduces potential deadlock risks: Using two different locking mechanisms (built-in
synchronizedand explicitReentrantLock) in the same method could lead to complex lock ordering issues and increase deadlock risks.Although the risk of deadlock in the current implementation is low, if someone modifies the code in the future and adds a method that acquires these two locks in a different order, it would introduce a deadlock risk.
Improves performance: Removing the unnecessary synchronization layer reduces lock contention and context switching overhead.
Code consistency: Other methods in the same class only use
ReentrantLockfor synchronization, so removingsynchronizedmakes the code style more consistent.More precise control:
ReentrantLockalready provides all the synchronization needed with more features than the implicitsynchronizedkeyword.Does this PR introduce any user-facing change?
No. This change is an internal implementation improvement that doesn't affect any public APIs or user-visible behavior.
How was this patch tested?
The existing tests for
ThreadUtilswere run to ensure that the behavior ofsameThreadExecutorService()remains unchanged. No additional tests were added as this is a straightforward removal of redundant synchronization that doesn't alter the method's behavior or contract.Was this patch authored or co-authored using generative AI tooling?
No.