Skip to content
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

PAYARA-3811: Optimize deployment times #4279

Merged
merged 6 commits into from
Oct 24, 2019
Merged

Conversation

pdudits
Copy link
Contributor

@pdudits pdudits commented Oct 22, 2019

Description

This is a feature.

Profiling of deployment of large EAR with tens of EJB module reveals, that significant amount of deployment time was spend in filesystem access, especially on Windows. Thousands of calls were made to File.exists() and File.isDirectory. This was caused by ASURLClassLoader, which delegated all of resource existence checks to filesystem and FileArchive doing recursive folder scanning.

ASURLClassLoader did have map, that should serve as cache, but it cached positive lookups, whereas negative lookups are more expensive -- because every possible class source is scanned in order, and therefore there may be tens of negative lookups until positive one is found. Also, the use of this map was disabled because of generated folders.

This PR replaces these hot code paths:

  • ASURLClassloader walks each class folder and caches up to three levels of subdirectories. This give precise enough hit rate to avoid file misses
  • Since some of the folders contained generated classes, which are constructed after classloader is created, it will watch for filesystem changes to extend its set of prefixes.
  • FileArchive uses file tree walking instead of recursive calls to file methods

Testing

New tests

DirWatcherTest tests the new decider for possibly valid files.

Testing Performed

Existing unit tests for FileArchive.

Ran against private large .ear with following results:

[Payara 5.191] [INFO] [] [javax.enterprise.system.core] [
app was successfully deployed in 72 787 milliseconds.]] 
[Payara 5.194-SNAPSHOT] [INFO] [] [javax.enterprise.system.core] [[
app was successfully deployed in 51 453 milliseconds.]] 

Test suites executed

Keeping that for Jenkins.

Testing Environment

Windows 10, 1.8.0_221, Maven 3.6.2

Notes for Reviewers

The PR only affects performance of directory-based classloaders (i. e. EJB generated folders, WEB-INF/classes), handling of JAR files is unaffected.

…sloaders

Every directory entry now caches valid prefixes. To support classes generated during
deployment, these directories are registered with WatchService in order to update
their data
This replaces recursive File calls, that were causing lots of slow filesystem I/O during
deployment of large projects
@pdudits pdudits requested a review from AlanRoth October 22, 2019 15:17
@pdudits
Copy link
Contributor Author

pdudits commented Oct 22, 2019

Jenkins test please

@pdudits
Copy link
Contributor Author

pdudits commented Oct 23, 2019

Jenkins test please

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

It would be nice if the file IO could be extracted to a black box util that just tells you "cached" if a certain file exists somewhere relative to a root folder. At least I'd assume this is the problem it solves.

*loader was created. Add it to the table of files we
*know about.
*/
File targetFile = privilegedCheckForFile(target);
Copy link
Contributor

Choose a reason for hiding this comment

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

privilegedCheckForFile is this some sort of Java accessibility check and if so should we keep having one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to tell because this codepath was not invoked for at least 8 years. But I can add privileged actions around filesystem access

Copy link
Contributor

Choose a reason for hiding this comment

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

I am happy to gamble on something that wasn't used in 8 years and not do any more than you already did. But that is just me. Don't know @Pandrex247 thinks about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see all TCK tests results on this, because classloading magic changed over time ... in SGES was strict classpath prefix and suffix, isolated untouchable endorsed, and fragile JNDI/classloading issues with EAR/WAR/EJB(outer/inner) scopes. Then came GF3 and everything changed. Javadoc of this class from 2011 says @since JDK 1.4, so I would expect it was something around SGES and perhaps even SJAS7 mechanisms.

Simply said: it is a zombie :D

Btw on line 826 of original file is incorrect string concatenation and also append("\n") is slower than append('\n'). And hasItem manipulates it's parameter - it is used in cycle so it does it again and again until JIT maybe would optimize it. Blah.

Copy link

@AlanRoth AlanRoth left a comment

Choose a reason for hiding this comment

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

LGTM

And remove no longer used method in ASURLClassLoader
@pdudits pdudits requested a review from jbee October 23, 2019 11:28
@pdudits
Copy link
Contributor Author

pdudits commented Oct 23, 2019

Jenkins test please

@pdudits
Copy link
Contributor Author

pdudits commented Oct 23, 2019

Jenkins test please

Copy link
Contributor

@jbee jbee left a comment

Choose a reason for hiding this comment

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

LGTM but then I have no idea what the classloaders where supposed to do so I cannot judge the semantics in the context.

@pdudits
Copy link
Contributor Author

pdudits commented Oct 24, 2019

Jenkins test please

1 similar comment
@pdudits
Copy link
Contributor Author

pdudits commented Oct 24, 2019

Jenkins test please

@pdudits pdudits merged commit adb46ec into payara:master Oct 24, 2019
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