-
Notifications
You must be signed in to change notification settings - Fork 134
Refine performance (2nd) #1492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Refine performance (2nd) #1492
Conversation
51bcd9b
to
7328df7
Compare
fix the bug about using MMap when value be collection, but the old codes didn't copy the collection, thus may cause appending to a list value in a DoneMMap, thus makes infinity long value, thus makes the run slow in super multi module project.
7328df7
to
f295e15
Compare
@cstamas please have a look. |
* | ||
* @since 1.9.0 | ||
*/ | ||
public static final int DEFAULT_THREADS = 5; | ||
public static final int DEFAULT_THREADS = Runtime.getRuntime().availableProcessors(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1: we can run on monsters as well. This is not an "improvement" on alg, just "throwing more" on it. In other words, doing this may hit "remote end" as well, think of https://en.wikipedia.org/wiki/Tragedy_of_the_commons like Central is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cstamas we increase the number because we try to merge the executers. see changes in several lines under that in the same file.
before, each instance have one seperated executor,and thus have 2 problems
- when running with like T2C, I afraid there might be very many of instance in parallel, thus many small executors in parallel
- the start and stop of the small executors be time costing.(especially adding worker threads)
Still,if you don't like mixing idea for merging the executors of every instance, we can remove this part. I just don't think it good to start many small thread executors, and I still think a bigger single executor is better in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we should use forkjoin pool by default, it is alsmot certainly created by a build these days and will avoid the overhead of custom pool with almost no benefit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we should use forkjoin pool by default, it is alsmot certainly created by a build these days and will avoid the overhead of custom pool with almost no benefit
yes it would be far better than using many small pools.
But as for forkjoin pool, I have no idea why we should use forkjoin pool but not executer here. IMO fork join pool is use for works who have relationship/dependency, and the tasks here seems not quite dependent on each other..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, nothing new since. It was just a faint idea, and did not move anywhere yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, nothing new since. It was just a faint idea, and did not move anywhere yet.
Um. Seems it is good enough a thing that worth my time. Would have a try when next weekends.
I agree on split it out of this pr, it is a bigger thing than my original thought.
Though I do agree it seems a better way than I provided in this pr.
Please focus on reviewing other part of this pr then.
…vers" This reverts commit 02691c4.
I think the intern logic (apart from the |
I've raised apache/maven#2495. I was quite sure the work was done already, but after investigation, it had not been plugged in. |
yes, I agree.
glad to see it. so I will remove this part from this pr too. |
…ng class in jdk." This reverts commit d0a0269.
252a8c2
to
82f1b17
Compare
refine performance for demo at apache/maven#2481
including 3 parts:
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verify
to make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify
).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.