This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
With the NetBeans IDE functionality getting richer and richer, our whitelist tests (see http://wiki.netbeans.org/FitnessViaWhiteAndBlackList) indicate that there are certain places where the infrastructure provided by the system is not scalable enough. E.g. places where an incremental slowdown with the amount of growing modules is inevitable. One of such area seems to be the extensible Lookup registration in Projects/project-type/Lookup/ layer directory. This style is responsible for activating functionality of webservices, profiler, persitance, hibernate and many more modules in case of simple JavaSE project being opened. This issue is created to estimate the potential gain achievable by improving the registration schema and also server as a blocker for issues reported against individual modules mentioned above.
Created attachment 71844 [details] A patch to help us measure the maximal boost we could get by making registrations more lazy
Created attachment 71845 [details] Module ide10/modules/org-netbeans-modules-projectapi.jar from release65 with the previous patch applied
as per discussion with jtulach, we agreed the only viable way to make lookup providers to lazy initialize, is to have a way to declaratively describe the trigger classes. when someone wants to get a class from the project's lookup, the infrastucture would match it against the declarative trigger classes and initialize the relevant lookupProvider instance. The whole process is tricky as the list of trigger classes needs to be kept in sync with the actual content. That's a major issue for long term maintainance. Please note that the system (with lazy loading based on triggers) is inherently MORE undeterministic and error prone. Additionally it's not clear how much will be actually gained in real-life examples. I'm sure the performance team will measure the cases where they can declare victory decisively. But we have previous examples (lazy loading of projects) that suggest that the performance issue need more holistic approach to performance tuning.
Created attachment 72507 [details] white list after applying the patch
On Tuesday 21 October 2008 22:46:29 Alexander Kouznetsov wrote: > The effect of this patch is as following: > > * 81 classes removed from startup with LimeWire project > * 3 issues fixed: bug 149553, bug 149554, bug 150127 > * 7 more issues partially fixed > * 1 issue would not be filed >
Created attachment 72508 [details] Module ide10/modules/org-netbeans-modules-projectapi.jar without the patch applied
Created attachment 72509 [details] Module ide10/modules/org-netbeans-modules-projectapi.jar with the patch applied, please compare this and the previous patch
Here are time measurements by Oleg: > start without a project : 25138ms delta: 0ms > start with JavaSE original: 29430ms delta: 4292ms > start with JavaSE patched : 28851ms delta: 3713ms This means, that the additional work caused by eager initialization of LookupProvider takes up to 14% of the JavaSE project opening overhead.
it has to be noted that your "patch" that you used to measure impact of LookupProviders, caused the project instance to be incomplete and possibly wrong! I also object to the term "eager initialization of LookupProvider". The LookupProviders are integral to the process of constructing a valid instance of project in the current context of the application. It seems that you are only trying to exaggerate the current "slowness" and at the same time the future possibilities of improvement to push your agenda forwards. A more precise and honest patch would 1. collect the creation times of each LookupProvider registered. 2. keep track of what gets looked up from project's lookup 3. report LookupProviders (with times) that contain *only* stuff that was never looked up. The sum of all such LookupProvider times is the MAXIMUM possible performance gain at project creation times. Minus the overhead of lazy instantiation management code of course. Additionally it has to be evaluated how much does the lazy instantiation management code slow down the regular project lookup calls.
I would add that under "overhead of lazy instantiation management code" should be counted the intrinsic overhead of having additional objects in the system filesystem that need to be loaded. Remember that if a _particular_ instance in project lookup is known to be intrinsically slow to load - this would generally be due to transitive class linking, since most services need do nothing in their constructors - the most straightforward fix, possible with no API change, would be to fix the LookupProvider to not use Lookups.fixed but rather an AbstractLookup that overrides beforeLookup to lazily create the instance. Of course there is still a 2-class overhead per module contributing to the project (LookupProvider + Lookup) but this is not a lot and may well be less than the overhead of a lazy lookup provider.
This issue, together with dependency on issue 149136, requests the AbstractLookup+beforeLookup+etc. to be standard for all registrations. As such, it needs to be easy to use standard and the details provided in previous Jesse's message need to be hidden. A simple annotation @ProjectServiceProvider(projectType="org-netbeans-modules-java-j2seproject",services=Hibernate.class) that would inside use the AbstractLookup+beforeLookup+etc. trick is imho good enough to eliminate junk like hibernate, persistence, spring, etc. and delay its initialization before it is really needed.
One problem is going to be with LookupMerger instances - while you only need a LookupMerger<ActionProvider> when you are looking up ActionProvider, even a "lazy" lookup provider is going to have to say it supports both ActionProvider and (the raw type) LookupMerger. If you don't declare LookupMerger, the project infrastructure might first look up LookupMerger and find nothing. But if you do declare LookupMerger, this provider will be loaded even when you look for a ClassPathProvider, and vice-versa, destroying some of the laziness. A possible solution: have the provider declare only ActionProvider, and ensure that the infrastructure looks up ActionProvider _first_ (forcing load of the provider), _then_ look up LookupMerger. For the case that the provider has a merger but no instances of that type to be merged (legal but probably unusual), it would just declare the LookupMerger and accept not being so lazy. Alternately, treat LookupMerger specially during registration. While an ActionProvider would become e.g. Projects/whatever-type/LookupByType/org.netbeans.spi.project.ActionProvider/my-LookupProviderImpl.instance (note that we _cannot_ use a subfolder of the originally requested path "Projects/whatever-type/Lookup" because Lookups.forPath would eagerly traverse it) a LookupMerger<ActionProvider> would become Projects/whatever-type/LookupMergerByType/org.netbeans.spi.project.ActionProvider/my-LookupProviderImpl.instance Either or both would be loaded whenever querying for ActionProvider.
An annotated class would need to have a public constructor. In general it may be necessary to pass a 'Lookup baseLookup' param. I would suggest that the annotation enforce that the class have exactly one public constructor with up to two parameters, a Project and/or a Lookup.
Created attachment 73788 [details] Start of a patch; have not yet figured out how to make LookupMerger's be loaded lazily when their mergeable class is first queried
The current patch makes no attempt to delay loading of the service interfaces themselves - which is no problem if the service is simply ClassPathProvider or similar, but a problem if the service is itself exotic, e.g. some Hibernate interface, or org.netbeans.modules.projectimport.eclipse.core.UpgradableProject. That part should be simple enough to solve: keep just the service name, not Class, in the constructor of LazyLookupProvider; and change boolean inited = false; // ... if (!inited && template.getType() == service) { inited = true; // ... } to Class<?> service; if (service == null && template.getType().getName().equals(serviceName)) { service = template.getType(); // ... } Similarly in proxyMergerLookup of course. The unit test could also verify that no service interface is loaded before you actually ask lookup for that service, by changing interfaces to abstract classes and registering them in loadedClasses just like the impls.
Thanks, this is what I was envisioning since beginning. Later I've started to speculate about eliminating individual .instance files - processing many of them is indeed not for free. Do you think we could change forProjectServiceProvider(Map<String,Object> attrs) to understand following registration? <file name="generalServices.instance"> <attr name="instanceCreate" methodvalue="....forProjectServiceProvider"/> <attr name="service.Type1" stringvalue="implof.Type1Impl"/> <attr name="service.Type2" stringvalue="implof.Type2Impl"/> ... </file> Such file could be generated per project type/implementation module. Or better, it could be generated just once per project type, and all modules would add attributes to it. The only issues are: can annotation processor add attributes for files generated by other modules and will those attributes be merged and visible? Also, how to support registration of multiple implementations of one service type?
"can annotation processor add attributes for files generated by other modules" - this would not be possible. Currently it is not even possible for an AP to add attributes to a file generated from another annotation in the same module, though this could be supported in LayerGeneratingProcessor if needed. I would not suggest such an approach, anyway. If there is indeed significant overhead associated with creating a few more InstanceDataObject's (which I would want to see measured), better would be to change LookupProviderSupport to scan named folders according to the lookup template it is handling.
I'd rather have a common lookupprovider implementation which would declaratively wrap around a singleton object. 1. the current registration space can be reused. 2. the annotation processing can list all interfaces/abstract classes for he singleton instance without clashes. 3. the the lookupprovider implementation would only be instantiated when the looked-up class is present in it. After that, it becomes live object and no further "lazy loading happens for that particular instance
If you're going to do specialized processing like this and still handle LookupMerger<T>, you will no longer be able to use Lookups.forPath. And the file can't really be a singleton object because the instance in general needs to be specific to a Project - so this also rules out InstanceCookie and FolderLookup. It's all possible, but I think you will find such a system to be more complex than my proposed patch, and more heavily reliant on Filesystems/Datasystems details.
I will try to work on it again.
Created attachment 75605 [details] Proposed API change
Please review this API change. I have attached a new patch which handles declarative LookupMerger registration, i.e. you can register a LookupMerger<SomeInterface> which will only be loaded if SomeInterface is queried. While originally I was trying to make the factory used internally by the annotation able to work without significant change to LookupProviderSupport, by creating proxy LookupMerger instances, I could not get this to work - something was wrong with the order of events, and the proxy LookupMerger was consulted too late to make a difference. Fortunately I was able to create a straightforward implementation with only a minor addition to what LPS searches for, and since the annotation is inside the same module this works without any impact on the public SPI. The new patch also avoids loading service interfaces, i.e. you can register a HibernateProviderImpl and be assured that HibernateProvider will not be loaded unnecessarily. Would need new spec vers, @since, apichanges.xml before committing. Currently the annotations can only be applied to class declarations, not factory methods. This could be improved now or in the future if there is any need (would be a compatible change). I am still collecting a list of good candidates for switching to the new annotations. Until today I thought I would simply check for all current users of @LookupProvider.Registration, but it seems there are some LookupProvider impls which are still registered manually. I will update these first and then try to create a summary. There are some modules which have somewhat complex LookupProvider implementations and it is not immediately clear whether these can be converted in a straightforward way.
The annotations also need an optional position attr, which should be easy to add. This seems to be missing from @LookupProvider.Registration, which I will correct now (seems to have been an oversight).
I guess the patch is OK. Can it sneak into main codebase for wider consumption by Jan 26, 2009?
(Sorry for delay.) I want to have at least some modules using @PSP when pushing the patch. My last quick inspection of sources using @LP.R showed them using some subtle idioms which cannot be converted trivially. I will have to look again more closely and make sure there are places where it can be used without loss of semantics.
Created attachment 76169 [details] New patch
Please check the updated patch. I'd like to commit it tomorrow (Friday) unless someone needs more time. I have tried to use the new annotations in as many places as possible, but there are still some places where @LP.R seems necessary, or where a conversion looked too complicated for anyone but the module owner to attempt. Notable changes in the API since last time: 1. As with @LP.R, you can either use String[] projectType for the common case, or @ProjectType[] projectTypes in case you want to specify positions. 2. Both new annotations may be used on factory methods rather than classes. For @PSP, the method may accept Project and/or Lookup args, just like the constructor. 3. You may specify >1 service interface (or abstract class). If you do, the service impl will be loaded the first time any of those interfaces is requested, and the same instance should be reused to satisfy all of the interfaces. Everything seems to compile, and the unit test passes, but I need to do some ad-hoc testing before commit.
The amount of removed code looks gorgeous. I guess you should use false in forNonWeb method.
Right, forNonWeb was wrong. I am also adding a unit test for semantics service={XFace1.class, XFace2.class}, and splitting off the new tests into LazyLookupProvidersTest for clarity.
Bear in mind that converting a LookupProvider to use @ProjectServiceProvider's does not automatically reduce the amount of class loading (other than for the LP itself). For example, GroovyActionProvider is still loaded when groovy.support is enabled and you open a j2seproject, since ActionProvider is being looked up. To enforce that most groovy.support classes not get loaded when a j2seproject not using Groovy is opened, you would in fact need to have a LP which does some quick check on the project - e.g. looking for a flag set in ProjectPreferences - and return Lookup.EMPTY whenever possible, keeping the number of loaded classes down to just one, the LP. Anyway there are a number of other groovy.support classes that get loaded even before you open the j2seproject. I think such problems could only be solved generally with something like issue #154653. So the primary benefit of @PSP is API usability; the performance benefit is probably marginal, limited to implementations of interfaces which are not usually requested. You can run for example: public final class TestAction implements ActionListener { public void actionPerformed(ActionEvent e) { System.err.println("===> running action"); for (Project p : OpenProjects.getDefault().getOpenProjects()) { Service s = p.getLookup().lookup(Service.class); if (s != null) { System.err.println("===> got a service: " + s.m()); } else { System.err.println("===> nothing for " + p); } } } public static abstract class Service { static { System.err.println("===> loading Service"); } public abstract String m(); } @ProjectServiceProvider(service=Service.class, projectType="org-netbeans-modules-java-j2seproject") public static class ServiceImpl extends Service { static { System.err.println("===> loading ServiceImpl"); } private final Project p; public ServiceImpl(Project p) { this.p = p; System.err.println("===> new ServiceImpl on " + p); } public String m() { return ProjectUtils.getInformation(p).getDisplayName(); } } } which if you open project Anagram2, run, then also open DesktopApplication1 and run, shows expected output: ===> running action ===> loading Service ===> loading ServiceImpl ===> new ServiceImpl on J2SEProject[/tmp/AnagramGame2] ===> got a service: AnagramGame2 ===> running action ===> got a service: AnagramGame2 ===> new ServiceImpl on J2SEProject[/tmp/DesktopApplication1] ===> got a service: DesktopApplication1
Also found & fixed a bug in @PSP applied to factory methods. core-main #8788d8cca3ec
Thanks for finishing and integrating the change. Re. your example: maybe you want to put it into wiki and link from javadoc to that wiki page. DevFAQUsingProjectServiceProvidersEffectively. Re. "primary benefit of @PSP is API usability" - Right. The performance impact is hard to express in ms, but you opened up more options for users of the API and there is no known negative impact. Plus there is slight improvement in classloading characteristics during start. I am happy that you did the change as "dobrá hospodyňka i pro pírko přes plot skočí". Thanks. Moreover the API is really nicer, simpler and more easy to use. Perfect example of where @annotations are handy.
Integrated into 'main-golden', will be available in build *200901241401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/8788d8cca3ec User: Jesse Glick <jglick@netbeans.org> Log: Issue #150194: add @ProjectServiceProvider and @LookupMerger.Registration as alternatives to @LookupProvider.Registration. These are potentially easier to use and more efficient than the older annotation. However, they do not apply to cases where the choice of lookup items must be determined dynamically.
I just added the example directly into Javadoc. core-main #018f45d9ceb5
Integrated into 'main-golden', will be available in build *200902020201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/018f45d9ceb5 User: Jesse Glick <jglick@netbeans.org> Log: #150194 addendum: adding example to Javadoc.