Skip to content

Conversation

@rjatkins
Copy link

@rjatkins rjatkins commented Mar 8, 2019

  • Greatly speeds up Windows filesystem interactions

 * Greatly speeds up Windows filesystem interactions
Copy link

@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.

Interesting.
Can we add some test case?

 * Opted to keep the no-args constructor for TrackingFileManager, so that DefaultUpdateCheckManager can continue creating throw away instances without any worries
 * Added a new configuration property for the TrackingFileManager fileLockCache maximum size, defaulting to 1024L
@rjatkins
Copy link
Author

I've added some unit tests around the cache, and I'm refreshing the performance results with the new cache, with the (new) default maximum size of 1024 entries. Note that Guava's caches never get to that limit, but use some heuristics to remove LRU entries before the cache gets full - this is for cases when the cache is being populated in multiple threads, when the cache would otherwise spend a lot of time in each thread purging the oldest entries.

@rjatkins
Copy link
Author

In local testing on Windows 10 (using jprofiler), with a particularly pathological use of maven-enforcer-plugin in a project with 23 bannedDependencies rules, across more than 100 poms, with a tree depth of 4, build times to run mvn validate come down from around 10 minutes to around 5 mins, shaving more than 300 seconds off the time spent executing java.io.File.getCanonicalPath.

This is when using maven-enforcer-plugin 3.0.0-M2, which happens to build a new dependency graph for each use of the bannedDependencies rule, so its performance is particularly bad for this project. I'm looking at also adding another higher level fix to that project that will speed things up considerably there too, although this performance fix in maven-resolver will still be valuable even after that's done.

@michael-o
Copy link
Member

How does this one compare to #22?

 * Avoids a good deal of further redundant I/O
@rjatkins
Copy link
Author

rjatkins commented Apr 1, 2019

#22 adds full caching around all uses of the properties files tracked by TrackingFileManager, while this PR was focused only on the calculations of canonical paths, where the bulk of the time is spent in my local testing. Caching all properties reads will give further performance improvements - my testing shows another 130s saved on my worst case maven project.
#22 uses an inline implementation of caching, while this PR uses Guava's CacheBuilder to maintain the cache for us, which includes thread-safety guarantees, and bulk expired entry cleanup after reads and writes. The canonical path cache in #22 uses an unbounded, never invalidated cache, while this PR ensures we only cache at most 1024 entries (and that's configurable). With the update I just pushed, it also caches at most 1024 properties objects too.
I have no strong opinion on either implementation - if you're willing to gain a code level dependency on Guava here, then I'm hoping it's easier to understand the code in this PR. Otherwise, the cache in #22 looks good enough. I'm just eager to see either of them merged, so we can all gain from the performance improvements.

 * Default to 1 minute, and allow configuration overrides with 1 second resolution
@michael-o
Copy link
Member

I believe that the TrackingFileManager suffers from the same problem as in MRESOLVER-114. See my understanding of the issue here. I started digging in the entire locking problem and the String#intern() is a bad design choice because that data is retained in memory.

@michael-o
Copy link
Member

@michael-o
Copy link
Member

Please evaluate: #67

@michael-o
Copy link
Member

Superseded by #67.

@jira-importer
Copy link

Resolve #856

@jira-importer
Copy link

Resolve #1080

@jira-importer
Copy link

Resolve #856

@jira-importer
Copy link

Resolve #1080

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.

4 participants