Unnecessary thread synchronization in RequestLoadSimulator?#4
Open
ams-tschoening wants to merge 1 commit intojohnrjenson:masterfrom
Open
Unnecessary thread synchronization in RequestLoadSimulator?#4ams-tschoening wants to merge 1 commit intojohnrjenson:masterfrom
ams-tschoening wants to merge 1 commit intojohnrjenson:masterfrom
Conversation
…ld be unnecessary because "invokeAll" returns only after all work has been done. The used locking to count reduced performance unnecessarily.
aaa1c31 to
5c88f50
Compare
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
1 participant
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 has already been "mentioned" in #2, but I think is worth some more discussion: Maybe I'm missing something, but in
RequestLoadSimulatoryou are counting the successfully processed requests and use locking on the class itself for that. The number of successfully processed requests seems to be only used as an abort condition.I think this is unnecessary, because "ExecutorService.invokeAll" is already defined to only return in case all threads successfully finished to provide their results.
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html#invokeAll(java.util.Collection)
We don't care about those results, though, but can simply remove the callback, locking and abort condition loop entirely. While the loop doesn't influence the runtime too much, the locking for incrementing the counter does have a higher negative impact.
During my executions of the same test with/without locking the results were pretty widespread, but without all the locking they were always larger than with.