Do not eager load non-eager module fragments. #4317
Conversation
sdedic
added
kind:bug
| * Release references to the ClassLoader. Package-private for now as only (?) FixedModule | ||
| * should retain the CL instance. | ||
| */ | ||
| void releaseClassLoader() { |
There was a problem hiding this comment.
Needed because I need to null classloader reference in fragments, yet do not want to bring their CL down using classLoaderDown. The fragment logic is centralized in ModuleManager - I do not want to pollute StadnardClassLoader.classLoaderDown().
| } | ||
| // remove m and m's fragments from parent classloaders, as fragment |
There was a problem hiding this comment.
remove(m) moved out of the loop, added fragments to prevent circularty.
| @@ -1793,6 +1838,13 @@ private boolean searchForPossibleEager(Set<Module> willEnable, Set<Module> still | |||
| continue; | |||
| } | |||
| if (m.isEager()) { | |||
| if (m.getFragmentHostCodeName() != null) { | |||
There was a problem hiding this comment.
Prevent eagers to 'just enable' if its host is not ... Maybe redundant, since if the eager uses Module-Dependency on its host, it would work as well.
| for (Module f : frags) { | ||
| List<Module> fragmentDep = fillMapSlot(m, f); | ||
| // move fragment after its host module in the sort order | ||
| fragmentDep.add(m1); | ||
| for (Dependency dep : f.getDependenciesArray()) { | ||
| if (dep.getType() == Dependency.TYPE_REQUIRES) { | ||
| Collection<Module> providers = providersOf.get(dep.getName()); | ||
| if (providers != null) { | ||
| if (providers.contains(m1)) { | ||
| providers = new ArrayList<>(providers); |
There was a problem hiding this comment.
removal of m1 moved out of the loop
|
|
||
| // m1 autoload, m2 normal, m3 eager | ||
| Module m1 = mgr.create(new File(jars, "host-module.jar"), null, false, false, false); | ||
| Module m2 = mgr.create(new File(jars, "fragment-module.jar"), null, false, false, false); | ||
| Module m2 = mgr.create(new File(jars, "fragment-module-reg.jar"), null, false, false, false); |
There was a problem hiding this comment.
adjusted m1, m2, m3 to match the comment
| ModsCreator c = new ModsCreator(); | ||
| createTestJAR(data, jars, "client-module-depend-host", "client-module"); | ||
| c.loadModules(); | ||
|
|
||
| Module client = c.mgr.create(new File(jars, "client-module-depend-host.jar"), null, false, false, false); | ||
| c.mgr.enable(client); | ||
|
|
||
| c.checkHostAndOtherFragmentsLoaded(c.host); | ||
| c.checkHostAndOtherFragmentsLoaded(); |
There was a problem hiding this comment.
This is the real change in behaviour: some fragments are no longer eagerly loaded (if they are not eager).
|
curious: did you measure how many modules are loaded less now? Or would this only occur during specific use cases? |
Not much of them:: But the change should eventually avoid bytecode load patching magic in the regular modules, since no "compatibility mode" (= patches) will be requested for them - unless the user activates a plugin that depends on an obsolete API (then a restart would be required). |
When starting NB IDE, I have noticed that
*-compat8module fragments are enabled - although they should not.commit-validationchecks that all dependencies in the distribution are up-to-date, so no module-auto-deps injection should injectcompat8module in standard NetBeans: they are intended for 3rd party plugins with obsolete (pre-9) dependencies.I have found that for some reason (already forgotten, and not documented in comments / commit messages), all registered fragments are enabled, whent he host module is enabled. This might be because if a fragment should enable later, after it's host's ClassLoader is already in use, the whole IDE would have to restart, since the ClassLoader's answers to
findResourcecalls might change and there's no way how to re-link already loaded classes.I changed the enabling process, and tested it on:
nbjavacloading. Nbjavac impl is a fragment hosted in nbjavac library. For testing purposes, I've checked out and built the old NB distribution with module system patched with these changes.The enable process now works that
autoload fragmentis enabled only because of dependency, or because it provides required or needed feature. Autoload fragments that are not requested are not loaded into the host.eager fragmentis enabled whenever all its dependencies are satisfied and its host is enabled.I have added testcases, that are based on the JavaFX scenario:
Summary of changes
In a dependency graph that is used to sort modules, the host module no longer depends on its fragments. Each fragment depends on its host. This ensures that host is enabled first, fragment modules are enabled later. Dependencies of fragments are injected into the host module, except hosting module itself and other fragments. Since the fragment's classloader items merge with the host, this dependency injection ensures that all libraries required by classes merged into host are enabled before the host module.
In a calculateParents all fragments are removed from the hosting module's parents, if they appear there. Fragment's dependencies are, to the contrary, added to the host's regular parents. This ensures that fragment classes injected into host's classloader load successfully (their library dependencies form parent classloaders) and that there's not circular delegation between host -> fragment -> host (a fragment typically depend on its host).