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

[NETBEANS-2940] Cache ArchiveRootProviders #1422

Merged
merged 1 commit into from Sep 10, 2019

Conversation

@lkishalmi
Copy link
Contributor

lkishalmi commented Aug 12, 2019

My intention was to improve the 40s search for ArchiveRootProviders shown at NETBEANS-2940

I'm not sure if this change good/safe enough. I'd ask the reporter to try this out first.
As far as I've checked it worked.

@lkishalmi

This comment has been minimized.

Copy link
Contributor Author

lkishalmi commented Sep 10, 2019

Well, if no one has comment against this, I'm going to merge this one tomorrow morning (PDT). As the change is pretty small this can be removed later if it would cause problems.

}
return res.allInstances();
return archiveRootProviderCache;

This comment has been minimized.

Copy link
@matthiasblaesing

matthiasblaesing Sep 10, 2019

Contributor

Why is this safe? If I'm reading it correctly after the first query no new ArchiveRootProvider will be found. So If at an unfortunate moment this is called before all ArchiveRootProviders are found (or they are loaded at runtime) this will return invalid results.

This comment has been minimized.

Copy link
@lkishalmi

lkishalmi Sep 10, 2019

Author Contributor

In Line 2335 we add a listener to the lookup result, so if new Archive providers would be registered in the system the cache would be invalidated and it's content would be re-read. Also so far there are 2 ArchiveRootProvider implementations in the system one for jars and one for jrt-s.
Profile snapshots attached to NETBEANS-2940 shows that NetBeans can spend considerable amount of time searching the non-jar provider almost every time when an archive root has to be evaluated.

This comment has been minimized.

Copy link
@matthiasblaesing

matthiasblaesing Sep 10, 2019

Contributor

Thank you for the explanation - indeed I overlooked the lookup listener and its invalidation implementation. I toyed a bit with the ProxyLookup and it looks to react correctly.
Looks sane to me now.

This comment has been minimized.

Copy link
@lkishalmi

lkishalmi Sep 10, 2019

Author Contributor

Thank you for the review! Let's see how does it work in the real life, then.

@lkishalmi lkishalmi merged commit 290420c into apache:master Sep 10, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@junichi11 junichi11 added this to the 11.2 milestone Sep 10, 2019
@arusinha-zz

This comment has been minimized.

Copy link

arusinha-zz commented Sep 19, 2019

hi @lkishalmi , I noticed an issue with this PR.
Once you install nb-javac plugin, you will not be able to run any java ant project in JDK-10/12/13.
There is no issue if the project is run using JDK-8.
Compilation of Sample project in JDK-10/12/13 have no issue, only it doesn't do anything on 'Run File' action
On reverting back the changes NB works fine. Request you to have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.