Improve performance using parallel streams for processes and threads#2204
Conversation
|
|
dbwiddis
left a comment
There was a problem hiding this comment.
Not sure if this was done,
Adds parallel on every stream call #2196
@dbwiddis I'm not actually sure when parallel should be used according to https://www.baeldung.com/java-when-to-use-parallel-stream so I have never actually used this method, is it necessary to use
.parallelon every stream call?
No, it's not necessary, or even desired to use it on every stream. From a performance perspective (the main reason for #2196) we want to balance the additional overhead of spawning/managing the streams, vs. the faster time from the parallel processing. And we don't want to use it in any place that order maters.
I've looked at every change and the vast majority aren't needed. Some are good. But you missed the primary reason for #2196 by only looking at existing streams.
For the below:
- keep the one for network local interface check
- (probably) keep the one for thread details, but there should probably also be ones for the other OS implementations
For things not included:
- please focus on methods that instantiate (a lot of) new OSProcess or new OSThread objects. Look at every getProcess() implementation on every OS. Some of them are in lists which could be turned into streams and parallelized.
- also look into the
getThreadDetails()implementations for the same reason. You found one... look at the others.
- also look into the
cef628b to
8b92846
Compare
dbwiddis
left a comment
There was a problem hiding this comment.
Great job, I love the improved readability and the performance will be good too.
Will approve with removal of star imports, but consider making the repeated valid thread check a constnat.
|
failing license check CI is due to copyright change date. Run mvn license:format to fix. |
dbwiddis
left a comment
There was a problem hiding this comment.
This looks really good, thanks for the updates! I've made a few more suggestions if you're willing to iterate one last time, otherwise this is probably GTG.
| : getAllNetworkInterfaces().stream().filter(networkInterface1 -> !isLocalInterface(networkInterface1)) | ||
| .collect(Collectors.toList()); | ||
| : getAllNetworkInterfaces().stream().parallel() | ||
| .filter(networkInterface1 -> !isLocalInterface(networkInterface1)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Huh, that networkInterface1 snuck in at some point, probably due to a refactoring. I like shorter names in lambdas, though, why not ni?
There was a problem hiding this comment.
hmmm how about just .filter(Predicate.not(AbstractNetworkIF::isLocalInterface))
There was a problem hiding this comment.
nevermind it seems has compilation errors
Error: /home/runner/work/oshi/oshi/oshi-core/src/main/java/oshi/hardware/common/AbstractNetworkIF.java:[159,42] cannot find symbol
| } | ||
| } | ||
| return threads; | ||
| Predicate<Map<PsThreadColumns, String>> columnPRI = threadMap -> threadMap.containsKey(PsThreadColumns.PRI); |
There was a problem hiding this comment.
(Preference) let's name this for the reason it exists, e.g., hasAllPSColumns
There was a problem hiding this comment.
would hasColumnsPri suffice? im not sure what AllPS means i.e. the other ones would be hasKeywordsArgs etc...
not actually sure what PRI means
|
Oh, and also a changelog entry would be nice! |
|
Spotless should help with the unused imports :) |
|
oops |
Sounds like a PR to fix spotless config is in order ;) |
dbwiddis
left a comment
There was a problem hiding this comment.
LGTM. Let me know if you're ready to merge this or still working.
yep its all good to go! |
Not sure if this was done,
Adds parallel on every stream call
Fixes #2196
@dbwiddis I'm not actually sure when parallel should be used according to
https://www.baeldung.com/java-when-to-use-parallel-stream
so I have never actually used this method, is it necessary to use
.parallelon every stream call?