Bug 169991 - Don't load classes until language is used & use @annotation to generate layer
Don't load classes until language is used & use @annotation to generate layer
Status: RESOLVED FIXED
Product: editor
Classification: Unclassified
Component: CSL (API & infrastructure)
6.x
All All
: P3 (vote)
: 6.x
Assigned To: issues@editor
issues@editor
perf-whitelist
: API, API_REVIEW_FAST
Depends on: 180081 180085
Blocks: 174846
  Show dependency treegraph
 
Reported: 2009-08-06 14:56 UTC by Jaroslav Tulach
Modified: 2010-02-03 21:55 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
In progress diff (30.01 KB, patch)
2009-11-10 05:32 UTC, mslama
Details | Diff
Bundle with Pavel's work done so far (149.73 KB, application/octet-stream)
2010-01-17 14:27 UTC, Jaroslav Tulach
Details
testWhitelist3 violators diff (4.56 KB, text/plain)
2010-01-27 09:26 UTC, Vitezslav Stejskal
Details
@PathRecognizerRegistration annotation added to parsing.api (122.53 KB, patch)
2010-01-27 09:35 UTC, Vitezslav Stejskal
Details | Diff
Additional CSL API cleanup (97.40 KB, patch)
2010-01-27 09:41 UTC, Vitezslav Stejskal
Details | Diff
Patch for contrib modules (12.47 KB, patch)
2010-01-31 16:13 UTC, Vitezslav Stejskal
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2009-08-06 14:56:58 UTC
Please rewrite the CslJar to @CslRegistration, delete most of the support code in CslJar.java and replace it with 
LayerGeneratingProcessor as advocated at
http://wiki.apidesign.org/wiki/CompileTimeCache

The goals:
1. move to standard infrastructure
2. delete a lot of useless code
3. simplify builds scripts (no need for special calls to special ant task)
4. improve performance (by registering a proxy with display name, list of mime types, etc.)
Comment 1 mslama 2009-11-10 05:32:27 UTC
Created attachment 90702 [details]
In progress diff
Comment 2 mslama 2009-11-10 05:42:13 UTC
M csl.api/nbproject/project.xml
M csl.api/src/org/netbeans/modules/csl/spi/DefaultLanguageConfig.java
M html.editor/src/org/netbeans/modules/html/editor/gsf/HtmlLanguage.java
A csl.api/src/org/netbeans/modules/csl/impl/LanguageProcessor.java
A csl.api/src/org/netbeans/modules/csl/impl/NoDeclarationFinder.java
A csl.api/src/org/netbeans/modules/csl/spi/LanguageRegistration.java
A csl.api/test/unit/src/org/netbeans/modules/csl/spi/LanguageRegistrationTest.java
Comment 3 Pavel Flaska 2009-12-01 01:59:53 UTC
Once fixed, back out 270d0d7fca12.
Comment 4 Quality Engineering 2009-12-02 02:58:54 UTC
Integrated into 'main-golden', will be available in build *200912020200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/270d0d7fca12
User: Pavel Flaska <pflaska@netbeans.org>
Log: #169991: Languages should not be loaded - temporarily allowed. Rollback once the bug is fixed.
Comment 5 Jesse Glick 2009-12-09 11:32:32 UTC
Seems pflaska is working on this in

http://hg.netbeans.org/ergonomics/rev/annotation-based-csl-169991

?

BTW I see you have declarations like this:

public Class/*<? extends OccurrencesFinder>*/ occurrencesFinder() default Void.class;

For best type safety and clarity you may want to write it as:

public Class<? extends OccurrencesFinder> occurrencesFinder() default NoOccurrencesFinder.class;

where NoOccurrencesFinder (if possible in an impl package) is assignable to OccurrencesFinder but does nothing (all methods assert false), and the processor would check for this value and skip the registration.
Comment 6 Pavel Flaska 2009-12-15 08:36:51 UTC
The declaration is prepared to work in a way you suggested, unfortunately there is a javac bug in 1.6, the code is not compilable. (Once I find it I will attach the bug id.) It works in 1.7.
Comment 7 Jan Lahoda 2010-01-14 10:32:14 UTC
The bug Pavel talked about is:
http://bugs.sun.com/view_bug.do?bug_id=6512707
fixed in JDK7 and OpenJDK6. I realized it might be possible
to workaround the bug on NB side (the bug is caused by insufficient
cleanup, and I think it should be possible to create an annotation
processor that would perform the missing cleanup instead of
the compiler). But, if we could ensure that we always build 
with a compiler that does not contain this bug, I wouldn't need
to implement this workaround. Is there a chance that we would
always use our own copy of javac to build NB (like we currently
do on JDK5), or should I try to create the workaround?
Comment 8 Jesse Glick 2010-01-14 12:35:19 UTC
(In reply to comment #7)
> Is there a chance that we would
> always use our own copy of javac to build NB (like we currently
> do on JDK5)

No. When Ant is run on JDK 6, we do not override javac. Maven users would be expected to use the standard javac. Etc.

> or should I try to create the workaround?

If possible, or just stay with an annotation declaration which does not trigger the bug to begin with.
Comment 9 Jaroslav Tulach 2010-01-15 14:55:33 UTC
Guys, thanks for your interest, I need your help. Pavel created a branch in ergonomics repository called annotation-based-csl-169991. He did not manage to finish his work, but as far as I know, he is able to generate most of the original Ant layer entries. Probably the biggest problem is that he generates slightly more.

Otherwise the issue is solvable. The limitations with JavaC compiler were fixed with few "catch", so we are not limited with JavaC bugs. The branch works with every JDK6 compiler.

My attempt is to finish the originally planned work of performance team for 6.9. This enhancement for 6.9 is still on my radar, but I will happily welcome any help you want to provide. If anyone of you wants to finish this branch, I'll be more than thankful.
Comment 10 Jan Lahoda 2010-01-15 15:25:59 UTC
Actually, I hit the javac bug in a different case (default severity of a hint, i.e. the default value of the attribute is an enum constant), for which I do not know any reasonable workaround. I am sorry, but it is very likely I could help with this issue except possibly with the workaround for the javac bug.
Comment 11 Jesse Glick 2010-01-15 15:59:36 UTC
(In reply to comment #9)
> I need your help.

Meaning me, or who?
Comment 12 Jaroslav Tulach 2010-01-17 14:27:29 UTC
Created attachment 93357 [details]
Bundle with Pavel's work done so far

Any help is welcomed. If I get none, I'll see what I can do on Jan 25, 2010.
Comment 13 Vitezslav Stejskal 2010-01-18 09:53:41 UTC
As a clear sign of weak mind I agreed to have a look at the patch and finish this work. I don't know much about annotation processors, but hopefully most of this stuff was done by Pavel and I'll just have to clean up the resulting layer registrations, which should not be that hard.
Comment 14 Vitezslav Stejskal 2010-01-20 09:53:11 UTC
Oh well, I had a look at the patch and I can't quite guess what exactly it was going to solve. So I went back to this issue to see what the problems/requirements are and don't seem to be able to guess that either. See below...

(In reply to comment #0)
> Please rewrite the CslJar to @CslRegistration, delete most of the support code
> in CslJar.java and replace it with 
> LayerGeneratingProcessor as advocated at
> http://wiki.apidesign.org/wiki/CompileTimeCache

I understand this.


> 
> The goals:
> 1. move to standard infrastructure
What is the standard infrastructure? Is it LayerGeneratingProcessor? If so, then this is just restating the above.


> 2. delete a lot of useless code
What code? The CslJar task? It's not useless at the moment. But can replaced with an annotation processor, so again probably restating the initial requirement.


> 3. simplify builds scripts (no need for special calls to special ant task)
Ok, this is the same as the initial requirement above.


> 4. improve performance (by registering a proxy with display name, list of mime
> types, etc.)
What exactly is meant by this? A proxy to what? What performance problems are we going to solve? Where are measurements demonstrating these problems?

As far as I can tell Pavel's patch can't work and the direction in which it seems to be going is dubious to say the least. The only requirement that I can see at the moment is to get rid of CslJar, which is legitimate and which I'm going to do in 6.9. IMO it can be done quite easily with much simpler annotation and with almost no impact on the language plugins.
Comment 15 Jaroslav Tulach 2010-01-21 02:44:33 UTC
Thanks for looking at Pavel's work, Víťo.

Yes, getting rid of CslJar is necessary initial step (it does not make much sense to improve CslJar task itself, better to fix new LayerGeneratingProcessor). That shall solve #1 (yes, layer generating processor is the standard infrastructure), #2 (you don't need the CslJar's XML manipulation code, keep just the "business logic" rebuilt on top of layer generating processor) and #3 (plus people that depend on CSL will be able to use incremental binary builds http://wiki.netbeans.org/AutoUpdateTask, which are broken right now).

As the #4 goal goes: We want to improve the registrations in layer to prevent classes of unused languages to be loaded. Pavel's changeset
http://hg.netbeans.org/main/rev/270d0d7fca12
shows the list of classes that inspired us to report this issue. The list is growing. The current set of additional violations is produced after every build of http://deadlock.netbeans.org/hudson/job/ergonomics/
and can be found at
http://deadlock.netbeans.org/hudson/job/ergonomics/lastSuccessfulBuild/artifact/ide.kit/build/test/qa-functional/work/o.n.t.i.W/testWhitelist3/whitelist_violators_3.txt
We hope that the @CslRegistration will allow us to make the violation list smaller and identify additional steps ultimately leading to complete elimination of loading CSL related classes of unused languages.
Comment 16 Vitezslav Stejskal 2010-01-22 06:26:57 UTC
The CslJar task is now history:
http://hg.netbeans.org/jet-main/rev/66c711b525f0
http://hg.netbeans.org/jet-main/rev/7a0adb1bb800

I'm leaving this open in order to lesser the number of loaded classes as requested by Jarda. I doubt I will be able to get rid of all of them, but the situation should get better when Parsing API provides an annotation for registering PathRecognizers.
Comment 17 Vitezslav Stejskal 2010-01-22 15:19:07 UTC
http://hg.netbeans.org/jet-main/rev/56bb0b6e32b3
Comment 18 Quality Engineering 2010-01-24 08:46:52 UTC
Integrated into 'main-golden', will be available in build *201001240200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/66c711b525f0
User: Vita Stejskal <vstejskal@netbeans.org>
Log: #169991: Adding @LanguageRegistration as a replacement for CslJar ant task; converting trunk build modules to use it
Comment 19 Vitezslav Stejskal 2010-01-27 09:26:19 UTC
Created attachment 93605 [details]
testWhitelist3 violators diff

The diff in the testWhitelist3 violators list after adding @PathRecognizerRegistration and cleaning up MimeLookup registrations.
Comment 20 Vitezslav Stejskal 2010-01-27 09:35:42 UTC
Created attachment 93606 [details]
@PathRecognizerRegistration annotation added to parsing.api

This patch contains the API change in parsing.api module, which adds @PathRecognizerRegistration annotation. It also contains changes in language plugins, which use this new annotation.
Comment 21 Vitezslav Stejskal 2010-01-27 09:41:16 UTC
Created attachment 93607 [details]
Additional CSL API cleanup

This patch contains changes in CSL API, which hides implementation details exposed in o.n.m.csl.core and o.n.m.csl.editor packages. The only classes in these packages that are needed by language plugins were several editor actions, which I moved to o.n.m.csl.api. This change plus cleaning up MimeLookup registrations in the previous patch also helped to decrease the number of classes loaded during the whitelist test.

The language plugins dependencies were updated accordingly and their spec versions were incremented in order to propagate this change through AUCs. I plan to do the same for contrib modules.
Comment 22 Vitezslav Stejskal 2010-01-27 09:49:46 UTC
Please review the API change in parsing.api, which adds new @PathRecognizerRegistration annotation. 

There is also an incompatible change in csl.api, which hides CSL's implementation accidentally exposed in o.n.m.csl.code and o.n.m.csl.editor packages. I'll update csl.api's apichanges.xml and the contrib modules before final push.

Thanks
Comment 23 Vitezslav Stejskal 2010-01-31 16:13:08 UTC
Created attachment 93693 [details]
Patch for contrib modules

... will be applied when the jet-main changes propagate.
Comment 25 Vitezslav Stejskal 2010-02-01 07:34:44 UTC
http://hg.netbeans.org/jet-main/rev/fa629ae8c625
Comment 26 Vitezslav Stejskal 2010-02-01 08:09:04 UTC
http://hg.netbeans.org/jet-main/rev/8cf7238b12f3
Comment 27 Vitezslav Stejskal 2010-02-01 15:25:48 UTC
http://hg.netbeans.org/jet-main/rev/a749d8abf225
Comment 28 Quality Engineering 2010-02-03 21:55:35 UTC
Integrated into 'main-golden', will be available in build *201002040200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/a749d8abf225
User: Vita Stejskal <vstejskal@netbeans.org>
Log: #169991: incrementing spec versions to fix the VerifyUpdateCenter.diachronicConsistency test failure after e2805e14bfc0


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo