Skip to content

Conversation

@AlexanderAshitkin
Copy link

@AlexanderAshitkin AlexanderAshitkin commented Oct 3, 2019

Synopsis
Synchronized method DefaultProjectBuildingHelper#createProjectRealm on project critical path invokes io under lock by calling ProjectRealmCache#createKey
That's restricts scaling of graph build horizontally by invoking more threads

Proposal:
Cache results of first file access to prevent subsequent io

Risks:

  • it is unclear why need to get file attributes each times. That could mean that file is supposed to mutate during the build which renders implementation incorrect

Options:

  • install file watcher to get notifications of file replacement
  • minimize synchronized section in DefaultProjectBuildingHelper#createProjectRealm - move ProjectRealmCache#createKey out of it

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MNG-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT successfully.

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.

@filipwiech
Copy link

Hi, any update on this?

@rfscholte
Copy link
Contributor

For this type of changes I would like to see an ICLA. I'd like to see the results with https://github.com/quick-perf/maven-test-bench to get the confirmation of this fix.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I left just a question

@AlexanderAshitkin
Copy link
Author

AlexanderAshitkin commented Feb 24, 2020

For this type of changes I would like to see an ICLA. I'd like to see the results with https://github.com/quick-perf/maven-test-bench to get the confirmation of this fix.

Robert, performance depends on file system. Slower FS - better numbers. In my case (rather slow virtual desktop which nowadays is guess a common case in enterprise worlds) results are:
Old -T1
[main] Phase 1: 1406 // phase1 - parse files
[main] Phase 2: 6539 // phase2 - process references
[main] Project graph built in: 7958

New -T1
[main] Phase 1: 1413
[main] Phase 2: 2450 // this is 4 seconds win for me each time i run any maven command

@michael-o
Copy link
Member

@AlexanderAshitkin Can you provide the exact steps? I will try on two completely different machines. One low power, another powerful HPE server.

@AlexanderAshitkin
Copy link
Author

AlexanderAshitkin commented Feb 25, 2020

@AlexanderAshitkin Can you provide the exact steps? I will try on two completely different machines. One low power, another powerful HPE server.

Hi
the steps are:

  1. in old implementation add timestamps of log graph build time - similar to [MNG-6774] - Speedup project graph build by paralleling operations #290 need time of DefaultProjectBuiler#build (or instrument it in any other preferable way)
  2. run new/old implementation on any reference project. result should be more distinctive on a bigger project.

for our 600-modules project difference is 13 vs 5 seconds. this could be further split in 2 phases - interim builds population and interim builds processing. The optimization helps to interim builds processing and win is 9 vs 1 sec for this piece in our case

@Dunemaster
Copy link

will someone merge it?

@maximilian-novikov-db
Copy link

@eolivelli could you please merge this

* Control flag to restore legacy filesystem behaviour in case issues.
*/
protected static class CacheKey
private final boolean useFsKey = Boolean.getBoolean( "maven.extensions.fskey" );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the need for this. It will only make it harder to maintain.

gnodet pushed a commit to gnodet/maven that referenced this pull request Nov 20, 2024
* [MNG-6847] Use diamond operator

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.staticanalysis.UseDiamondOperator?organizationId=QXBhY2hlIE1hdmVu

Co-authored-by: Moderne <team@moderne.io>

* Restore core-it-suite/src/test/resources/mng-5581-lifecycle-mapping-delegate/extension/src/main/java/org/apache/maven/its/mng5581/lifecyclemappingdelegate/TestLifecycleMappingDelegate.java

---------

Co-authored-by: Moderne <team@moderne.io>
@jira-importer
Copy link

Resolve #8447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants