Community
Participate
Working Groups
I still find the caching behavior of ModuleFactory troublesome for the following reason: The nature of the server infrastructure seems to require that the factory know every module at once, or else a DeletedModule will be created in its place. This goes against the principle of lazy initialization, requiring the factory to fully discover any and all modules it could possibly require. The API for these classes also only provides one catch-all getModules() method, instead of a more versatile and lazy-load capable method such as getModule(String id) which could return null if the factory doesn't have it or can't create it. Such an API would allow a factory to create the module upon request, rather than having to search for anything it would consider a module way in the beginning. For my factory's implementation, this causes a full search of a workspace for resources that may be modules, which is very time-consuming. It seems most Modules are created either by the factory of their own volition, or at the behest of the server framework when searching through the list of a Server's modules and creating them. (See Server.getModules()). The Server's getModules() method makes the following call: IModule module = ServerUtil.getModule(moduleId); This separates the module into a factoryId and a sub-name, gets a reference to the factory, and asks for the module of the sub-name. This is *good* and seems to work with lazy initialization. (IModule module = moduleFactory.getModule(moduleSubId);) However, the code for getModule(String id) is as follows: public IModule getModule(String id) { IModule[] modules2 = getModules(); if (modules2 != null) { int size = modules2.length; for (int i = 0; i < size; i++) { Module module = (Module) modules2[i]; if (id.equals(module.getInternalId())) return module; } } return null; } public IModule[] getModules() { //Trace.trace(Trace.FINER, "getModules() > " + this); if (modules == null) { try { modules = new ArrayList(); IModule[] modules2 = getDelegate(null).getModules(); if (modules2 != null) { int size = modules2.length; for (int i = 0; i < size; i++) modules.add(modules2[i]); } } catch (Throwable t) { Trace.trace(Trace.SEVERE, "Error calling delegate " + toString() + ": " + t.getMessage()); return null; } } //Trace.trace(Trace.FINER, "getModules() < " + this); IModule[] m = new IModule[modules.size()]; modules.toArray(m); return m; } This, clearly, requests a full cache from the delegate, and if the delegate is trying to go along with lazy initialization, it will probably return an empty or pruned array. At no point is it even possible for the factory to ask its delegate for a specific module ID. This lack of API requires a full initialization instead of a lazy load. But the API alone isn't the problem... even if it were simply added, that wouldn't solve the problem unless the webtools infrastructure made a concerted effort to use the lazier initialization as much as possible rather than using blanket requests for all of a factory's modules.
Agreed. We used to have a getModule(String id) method and it was removed due to other problems. We should bring it back and will also need to investigate if there are other ways to lazily load modules. Currently, there are too many places that ask for the full list of modules, ruining any chances of improving the overall situation.
I've reconfigured my factory in the following fashion. The problem here is that I use non-public API, specifically access the Server class, as well as a protected static attribute key which I shouldn't have access to. This is obviously less than ideal. Overall, the goal of this edit is to get a list of all servers' modules BEFORE I'm asked for all my modules as a factory. In this way I know (or think I know) what modules MUST be initialized immediately, specifically those that are going to be required by the server's for the ui. Perhaps this idea could be extrapolated into an API where classes can register themselves as early requirers of modules, ie that they keep references to module IDs which will need to be loaded early or at least prepared to be loaded. In my below impl, most of the code was stolen from the Server class, so it'd also be interesting if there were a public way to get a list of module ID's instead of just IModule[] getModules(); --- protected void cacheModules() { String[] paths = getServerModulePaths(); for( int i = 0; i < paths.length; i++ ) { acceptAddition(paths[i]); } } private String[] getServerModulePaths() { // Stolen from Server.class, not public final String MODULE_LIST = "modules"; ArrayList paths = new ArrayList(); IServer[] server = ServerCore.getServers(); for( int i = 0; i < server.length; i++ ) { if( server[i].getClass().equals(Server.class)) { // Get the module ID list: Object[] o = ((Server)server[i]). getAttribute(MODULE_LIST, (List)null).toArray(); // Get the module ID list: for( int j = 0; j < o.length; j++ ) { String moduleId = (String) o[j]; String name = "<unknown>"; int index = moduleId.indexOf("::"); if (index > 0) { name = moduleId.substring(0, index); moduleId = moduleId.substring(index+2); } String moduleTypeId = null; String moduleTypeVersion = null; index = moduleId.indexOf("::"); if (index > 0) { int index2 = moduleId.indexOf("::", index+1); moduleTypeId = moduleId.substring(index+2, index2); moduleTypeVersion = moduleId.substring(index2+2); moduleId = moduleId.substring(0, index); } if( moduleId.startsWith(getFactoryId() + ":")) { String path = moduleId.substring((getFactoryId()+":").length()); paths.add(path); } } } } return (String[]) paths.toArray(new String[paths.size()]); }
I'm going to use this bug to add new API to query modules by IProject and moduleId and push this through the server tools API and as much of the UI and tools as possible. This should allow lazy initialization in as many cases as possible, but there are still a couple cases (e.g. Add/Remove modules dialog) which currently need to load all modules of a particular type(s). (any ideas to improve that situation are welcome :) ) I'm not sure I want to comment on the modifications to Server since that seems to get further away from the current API and I don't have a clear vision of how it would be implemented. Unless you have strong objections I think the API changes listed above are natural extensions to the current API that we should implement now, and any other changes should be pushed back and handled via another bug if they are still required after validating these changes.
That sounds great. I have module factories which need to be able to load a module of a certain ID, but also don't want to be shown in the add-remove projects dialog or return a "complete" list of modules. If the API you suggest is good enough (and forced through / used as much as possible) I can choose to ignore requests to get a full module list, while still responding to specific requests. That's my use-case at least.
Changes complete. Did several reference searches and there are now only two exposed paths that will end up calling the full getModules(): ServerUtil.getModules(IModuleType[] moduleTypes) // returns all modules of an array of types - Only used by Add/Remove modules action ServerUtil.getModules(String moduleType) // returns all modules of a given type - Only used by jst.j2ee utility to return parent modules (e.g. return all web modules that contain a given utility jar) The first one is fairly contained and unavoidable, but the second may be used more often. For instance, Tomcat uses the utility in getRootModules() when checking if a utility modules is contained within a web module. Note that I have no control over who calls this API from other components (e.g. Web Services) or in adopter products, nor any good way to find these references. I've added javadoc to these methods that's clear about the performance impact. I plan to release this fix next week since we're currently in I-build shutdown.
Changes released to 2.0 M5 builds.
Old bug, closing.
New Gerrit change created: https://git.eclipse.org/r/108136
New Gerrit change created: https://git.eclipse.org/r/108137
New Gerrit change created: https://git.eclipse.org/r/109677
Gerrit change https://git.eclipse.org/r/109677 was merged to [master]. Commit: http://git.eclipse.org/c/servertools/webtools.servertools.git/commit/?id=8800319f4b2ac39bd6e0f8de3fe093e042506384
