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.

Bug 150194 - LookupProvider registration is not scalable
Summary: LookupProvider registration is not scalable
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Generic Infrastructure (show other bugs)
Version: 6.x
Hardware: All All
: P3 blocker (vote)
Assignee: Jesse Glick
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 149136 166910
Blocks: 148178 149564 157283
  Show dependency tree
 
Reported: 2008-10-15 11:03 UTC by Jaroslav Tulach
Modified: 2009-06-11 18:34 UTC (History)
7 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
A patch to help us measure the maximal boost we could get by making registrations more lazy (745 bytes, patch)
2008-10-15 11:41 UTC, Jaroslav Tulach
Details | Diff
Module ide10/modules/org-netbeans-modules-projectapi.jar from release65 with the previous patch applied (205.65 KB, application/x-compressed)
2008-10-15 11:42 UTC, Jaroslav Tulach
Details
white list after applying the patch (19.25 KB, text/plain)
2008-10-23 07:34 UTC, Jaroslav Tulach
Details
Module ide10/modules/org-netbeans-modules-projectapi.jar without the patch applied (218.09 KB, application/x-compressed)
2008-10-23 07:36 UTC, Jaroslav Tulach
Details
Module ide10/modules/org-netbeans-modules-projectapi.jar with the patch applied, please compare this and the previous patch (218.04 KB, application/x-compressed)
2008-10-23 07:38 UTC, Jaroslav Tulach
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 (24.22 KB, patch)
2008-11-15 21:08 UTC, Jesse Glick
Details | Diff
Proposed API change (32.13 KB, patch)
2009-01-09 00:24 UTC, Jesse Glick
Details | Diff
New patch (132.14 KB, patch)
2009-01-23 01:58 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2008-10-15 11:03:08 UTC
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.
Comment 1 Jaroslav Tulach 2008-10-15 11:41:29 UTC
Created attachment 71844 [details]
A patch to help us measure the maximal boost we could get by making registrations more lazy
Comment 2 Jaroslav Tulach 2008-10-15 11:42:47 UTC
Created attachment 71845 [details]
Module ide10/modules/org-netbeans-modules-projectapi.jar from release65 with the previous patch applied
Comment 3 Milos Kleint 2008-10-15 12:03:21 UTC
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.


Comment 4 Jaroslav Tulach 2008-10-23 07:34:20 UTC
Created attachment 72507 [details]
white list after applying the patch
Comment 5 Jaroslav Tulach 2008-10-23 07:35:24 UTC
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
>
Comment 6 Jaroslav Tulach 2008-10-23 07:36:32 UTC
Created attachment 72508 [details]
Module ide10/modules/org-netbeans-modules-projectapi.jar without the patch applied
Comment 7 Jaroslav Tulach 2008-10-23 07:38:31 UTC
Created attachment 72509 [details]
Module ide10/modules/org-netbeans-modules-projectapi.jar with the patch applied, please compare this and the previous patch
Comment 8 Jaroslav Tulach 2008-10-24 13:50:37 UTC
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.
Comment 9 Milos Kleint 2008-10-24 14:09:38 UTC
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.


Comment 10 Jesse Glick 2008-10-24 22:48:34 UTC
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.
Comment 11 Jaroslav Tulach 2008-10-29 15:39:38 UTC
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.
Comment 12 Jesse Glick 2008-10-30 16:18:37 UTC
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.
Comment 13 Jesse Glick 2008-11-15 17:56:04 UTC
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.
Comment 14 Jesse Glick 2008-11-15 21:08:34 UTC
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
Comment 15 Jesse Glick 2008-11-15 21:22:49 UTC
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.
Comment 16 Jaroslav Tulach 2008-11-19 09:49:54 UTC
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?
Comment 17 Jesse Glick 2008-11-20 20:23:39 UTC
"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.
Comment 18 Milos Kleint 2008-11-24 08:55:35 UTC
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

Comment 19 Jesse Glick 2008-11-24 18:27:16 UTC
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.
Comment 20 Jesse Glick 2009-01-08 15:03:03 UTC
I will try to work on it again.
Comment 21 Jesse Glick 2009-01-09 00:24:04 UTC
Created attachment 75605 [details]
Proposed API change
Comment 22 Jesse Glick 2009-01-09 00:35:38 UTC
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.
Comment 23 Jesse Glick 2009-01-09 00:43:51 UTC
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).
Comment 24 Jaroslav Tulach 2009-01-21 09:41:39 UTC
I guess the patch is OK. Can it sneak into main codebase for wider consumption by Jan 26, 2009?
Comment 25 Jesse Glick 2009-01-21 14:06:12 UTC
(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.
Comment 26 Jesse Glick 2009-01-23 01:58:05 UTC
Created attachment 76169 [details]
New patch
Comment 27 Jesse Glick 2009-01-23 02:03:44 UTC
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.
Comment 28 Jaroslav Tulach 2009-01-23 07:36:57 UTC
The amount of removed code looks gorgeous. 

I guess you should use false in forNonWeb method.
Comment 29 Jesse Glick 2009-01-23 16:06:18 UTC
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.
Comment 30 Jesse Glick 2009-01-23 16:34:57 UTC
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
Comment 31 Jesse Glick 2009-01-23 17:54:03 UTC
Also found & fixed a bug in @PSP applied to factory methods.

core-main #8788d8cca3ec
Comment 32 Jaroslav Tulach 2009-01-24 12:25:38 UTC
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.
Comment 33 Quality Engineering 2009-01-24 18:51:59 UTC
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.
Comment 34 Jesse Glick 2009-02-01 18:03:35 UTC
I just added the example directly into Javadoc. core-main #018f45d9ceb5
Comment 35 Quality Engineering 2009-02-02 08:14:11 UTC
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.