Created an attachment (id=71844) [details]
A patch to help us measure the maximal boost we could get by making
registrations more lazy

Created an attachment (id=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 an attachment (id=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 an attachment (id=72508) [details]
Module ide10/modules/org-netbeans-modules-projectapi.jar without the patch
applied

Created an attachment (id=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 an attachment (id=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 an attachment (id=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 an attachment (id=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.