Bug 150875 - Too many editor Action classes loaded at startup/warmup
Too many editor Action classes loaded at startup/warmup
Status: RESOLVED FIXED
Product: editor
Classification: Unclassified
Component: Actions/Menu/Toolbar
6.x
All All
: P3 (vote)
: 6.x
Assigned To: issues@editor
apireviews
: API, API_REVIEW_FAST, PERFORMANCE
: 162610 (view as bug list)
Depends on: 199823
Blocks: 144426
  Show dependency treegraph
 
Reported: 2008-10-21 15:10 UTC by Jaroslav Tulach
Modified: 2011-07-01 14:12 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
grep "Action$" all-loaded-classes.txt (24.03 KB, text/plain)
2008-10-21 15:11 UTC, Jaroslav Tulach
Details
method to refactor (294.66 KB, image/png)
2009-03-11 22:14 UTC, myopic4141
Details
Method after control r (294.55 KB, image/png)
2009-03-11 22:15 UTC, myopic4141
Details
Method with characters to delete highlighted (295.22 KB, image/png)
2009-03-11 22:18 UTC, myopic4141
Details
Method after command x (296.50 KB, image/png)
2009-03-11 22:19 UTC, myopic4141
Details
Method with characters to replace highlighted (296.81 KB, image/png)
2009-03-11 22:20 UTC, myopic4141
Details
Result after command v (293.96 KB, image/png)
2009-03-11 22:21 UTC, myopic4141
Details
Diff of the @EditorActionRegistration related changes (166.41 KB, patch)
2009-03-30 14:43 UTC, Miloslav Metelka
Details | Diff
Patch file after Yarda's suggestions (179.24 KB, patch)
2009-04-07 17:31 UTC, Miloslav Metelka
Details | Diff
Patch #3 (168.57 KB, patch)
2009-04-09 00:39 UTC, Miloslav Metelka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2008-10-21 15:10:38 UTC
I've been working on issue 133009 recently. I've listed all loaded classes after startup and did "grep Action$". I was 
expecting a lot of useless classes from shortcuts folder, menu and toolbar, but to my surprise about 40% came from 
editor. 

According to pnejedly, one loaded class occupies 3KB at least. With ~170 loaded classes this means 500KB of memory 
taken without really doing nothing. All these actions seem to subclass the same BaseAction class, which does not have 
any isEnable logic, just different actionPerformed. I'd like the editor team to somehow wrap these classes and either 
instantiate them lazily or not at all.
Comment 1 Jaroslav Tulach 2008-10-21 15:11:23 UTC
Created attachment 72393 [details]
grep "Action$" all-loaded-classes.txt
Comment 2 Vitezslav Stejskal 2008-10-22 12:18:44 UTC
I vaguely remember that this was already mentioned somewhere - maybe we don't need to warm up editors during startup
anymore. Pretty much all the editor actions are created from BaseKit.createActions when a kit is installed to a
JTextComponent for the first time. Currently we have a warm up task, which creates a fake JEditorPane for text/x-java
and this obviously also creates editor actions.

It should be easy to disable editor's and java editor's warm up tasks by simply commenting out the content that these
two modules (editor, editor.java) add to the "WarmUp" folder on the system filesystem. Jardo could you please do that
and see if it reduces the number of editor action classes loaded during startup? Thanks
Comment 3 Jaroslav Tulach 2008-10-24 09:34:41 UTC
Well, it can reduce the classes loaded during start, but not when the kit is initialized. All those 
RemoveWordPreviousAction
MoveSelectionElseLineUpAction
RemoveLineAction
BeginWordAction
RemoveTrailingSpacesAction
SelectIdentifierAction
InsertSemicolonAction
and their ~200 friends (rough guess) would still be loaded. Regardless of the fact that they are not used by our users 
at all. Moreover the rest of the actions, those that can be used, are still occupying way to much memory. Is it really 
necessary to have PageUpAction and PageDownAction as separate classes? Can't you have one MoveAction for all the caret 
moves parametrized with direction?
Comment 4 Vitezslav Stejskal 2008-10-24 11:03:33 UTC
> Well, it can reduce the classes loaded during start, but not when the kit is initialized

Oh, so the goal here is to have less classes implementing editor actions. And not to load less action classes during
startup. Is this correct? If so, then we should change the summary of this issue and obviously my previous comment makes
only little sense.


> Is it really necessary to have PageUpAction and PageDownAction as separate classes? Can't you have one MoveAction
> for all the caret moves parametrized with direction?

Maybe it's not necessary. Maybe we could aggregate some actions in one implementation class. What we have now was done
ages ago. Most of our action classes are publicly accessible and some of them are subclassed in other modules, which may
limit our options or complicate things.
Comment 5 Jaroslav Tulach 2008-10-29 15:13:44 UTC
OK, I've been thinking about the problem for few days, and here is slightly different style of fixing the problem:

I do not want you to sacrifice your object oriented coding style. Keep as many classes as you wish (although having 
one class for PgUp and one for PgDown is still an overkill from my point of view). However do not load and instantiate 
them during start/warmup. Return a EditorActionProxy that knows how to instantiate the right class when it is invoked.

You might also consider declaring your actions in layer, potentially with the use of support from issue 149136. Can 
this be done for 7.0?
Comment 6 myopic4141 2009-03-11 22:14:03 UTC
Created attachment 78077 [details]
method to refactor
Comment 7 myopic4141 2009-03-11 22:15:48 UTC
Created attachment 78078 [details]
Method after control r
Comment 8 myopic4141 2009-03-11 22:18:03 UTC
Created attachment 78079 [details]
Method with characters to delete highlighted
Comment 9 myopic4141 2009-03-11 22:19:18 UTC
Created attachment 78080 [details]
Method after command x
Comment 10 myopic4141 2009-03-11 22:20:29 UTC
Created attachment 78081 [details]
Method with characters to replace highlighted
Comment 11 myopic4141 2009-03-11 22:21:32 UTC
Created attachment 78082 [details]
Result after command v
Comment 12 myopic4141 2009-03-11 22:26:00 UTC
Something got messed up because the attachments were suppose to go to 150139.
Comment 13 Miloslav Metelka 2009-03-30 14:42:32 UTC
So I have created org.netbeans.api.editor.EditorActionRegistration in editor/lib2 to support lazily created actions. The annotation processor for that 
annotation generates creation of AlwaysEnabledAction into "Editors/<mime-type>/Actions" which ensures that these actions will be instantiated 
automatically by already existing editor infrastructure. I had to add certain additional properties into AEA:
  Action.SHORT_DESCRIPTION (i.e. "ShortDescription")
  "menuText"
  "popupText"

Editor actions are specific in that their Action.NAME property holds non-localized name e.g. "default-typed" and their localized description is in 
Action.SHORT_DESCRIPTION property. To eliminate necessity for double-populating the AEA (by annotation) and the delegate action itself all the above 
properties together with Action.NAME property are automatically synchronized into the delegate action once it gets created.

The following actions are not suitable for annotation based declaration:
1) If they use putValue() for other properties than those mentioned above.
2) If they override BaseAction.createDefaultValue() for some property not mentioned above.
3) If they change its enabled status dynamically.

The annotation based registration requires non-parameter constructor. If a particular editor action can be constructed with multiple names and certain flag 
e.g. "boolean selected" defines its behavior then typically the action's body can be rewritten to derive the "boolean selected = second-
name.equals(getValue(Action.NAME))" and then e.g.
  @EditorActionRegistration(name = { BaseKit.wordMatchNextAction, BaseKit.wordMatchPrevAction }, ... )
can be used.

Once the action is registered by annotation its creation in overriden BaseKit.createActions() should be removed.

Since the action changes editor APIs I would like to ask for fasttrack review. Thanks.
Comment 14 Miloslav Metelka 2009-03-30 14:43:59 UTC
Created attachment 79057 [details]
Diff of the @EditorActionRegistration related changes
Comment 15 Jaroslav Tulach 2009-03-30 21:14:24 UTC
Y01 When I compare RemoveTabAction and RemoveWordPreviousAction I am surprised that RTA passes name to the super 
constructor. Is that intended?

Y02 org.netbeans.editor.ActionFactory is in public package. By making its nested classes public, you are in fact 
making them part of API. Intentional? OK, if it is, however maybe you want to move the to some impl subpackage?

Y03 ChangeCaseAction constructor can be private

Y04 actionNameUpdate is missing @since tag

Y05 <change> shall refer to the javadoc. Either <a href="@TOP@/...."/> in HTML of <class name="..."/> in the change 
structure

Y06 It would be nice to have a test to show that the annotation processor can detect some faulty registration. Like 
class not being public, not having default contructor, etc.

Y07 What is LazyEditorAction good for? Relict of early design not needed anymore?

Y08 I guess you want to uncomment new lines in blacklist.txt. Potentially add more classes.

Y09 openide.awt needs spec version increment and change note, potentially also improvement to javadoc of alwaysEnabled 
method.

Y10 editor shall depend on new version of openide.awt


Thanks a lot for your magnificent patch!

Comment 16 Jaroslav Tulach 2009-04-03 13:31:33 UTC
Can we consider this patch for 6.7, please?
Comment 17 Miloslav Metelka 2009-04-07 17:29:57 UTC
Apologies, I forgot previously to reassign to apireviews alias.

Y01 and Y02:
It's because some actions have fixed instance creation and so if they would no longer have Action.NAME value populated in their constructor, they would 
stay "unnamed":
    private static final Action removeTabActionDef = new ActionFactory.RemoveTabAction();
A similar problem is that the actions are public and they are in public package so any other kit can extend them. If BaseKit's action would only be declared 
with registration annotation and there would be a target kit extending the action (not creating it by annotation but instead in createActions() ) it expects 
that the super-action is already named (in its constructor) which would not be true. In practice there are just few actions that are overriden in this way and 
to prevent modifications of target kits I have instead left some actions' their names filling in constructors.
 Of course a desired state is to not have the actions (registered by annotation) in public packages and instead of overriding actions' functionality have an 
appropriate SPI for that and leave the actions final.
I have added a check into BaseKit that all the created actions are named which should prevent extending of unnamed actions and not declaring them by 
registration annotation.

Y03 - Y10:
done.

Attaching new patch (in addition I must check blacklisted classes before integration).
Comment 18 Miloslav Metelka 2009-04-07 17:31:39 UTC
Created attachment 79669 [details]
Patch file after Yarda's suggestions
Comment 19 Jesse Glick 2009-04-07 18:16:44 UTC
[JG01] I would recommend just deleting comments like

// annotation-registration        actions.add(new ExtDefaultKeyTypedAction());

since they will grow out of synch with the actual registrations.


[JG02] If you have something in cp.extra, it should not need to be duplicated in test.unit.run.cp.extra, nor do you need
to have an empty test.unit.cp.extra.


[JG03] The annotation should specify @Retention(SOURCE) and whatever @Target is appropriate (TYPE + METHOD I suppose).


[JG04] The whole long switch (e.getKind()) {...} block and

if (methodName != null) {
    file.methodvalue("delegate", className, methodName);
} else {
    file.newvalue("delegate", className);
}

could likely be replaced with simply

file.instanceAttribute("delegate", Action.class);


[JG05] It seems likely that most of the bundle-handling code in the processor could be deleted and/or moved to
LayerBuilder and/or avoided by being more explicit in the annotation (*) about which style of label you are using.

(*) You can define annotations with no @Target which can be used as nested structure elements.


[JG06] It is not necessarily an error if the delegate for Actions.alwaysEnabled does not match the static registration
in all attributes. A delegate may wish to provide e.g. a dynamically determined display name, which begins updating live
only once the delegate is loaded. So I don't think syncActionDelegateProperty should issue warnings. (Perhaps at FINE
and you can look for them if you are not expecting any mismatches.)


[JG07] The MnemonicsTest.java patch looks independent, please commit separately.
Comment 20 Miloslav Metelka 2009-04-09 00:36:15 UTC
ad JG01:
Not a strong opinion so I've removed them.

ad JG02:
That was just because there's something wrong in my setup and AnnotationProcessorTestUtils.runJavac() still fails for me with
no JSR 199 compiler impl found...
so I've tried other classpaths as well but it does not help so I've removed extra classpaths. Even other modules' tests using the runJavac() (e.g. project.ant 
and settings) fail in my Mac env too :(

ad JG03:
Done.

ad JG04:
Not done yet but IMHO can be done anytime so I will possibly update it later (I need to integrate soon so that the patch gets tested thoroughly).

ad JG05:
Not sure whether I understand it but anyway I've made two annotations: EAR only allowing a single "String name();" and having EARs having "EAR[] value();" 
which simplifies the EARProcessor somewhat.

ad JG06:
For editor actions it indicates an error to have a different name in the delegate but it's ok to only log at FINE level so I've changed it.

ad JG07:
ok.
Comment 21 Miloslav Metelka 2009-04-09 00:39:14 UTC
Created attachment 79793 [details]
Patch #3
Comment 22 Jesse Glick 2009-04-09 14:59:04 UTC
JG02 - please file bugs for the appropriate module if you come across problems like this.


JG03 - SOURCE, not RUNTIME as you have here.


JG05 - @EditorActionRegistrations is probably a good approach but I can't confirm since EditorActionRegistrations.java
is not in the patch.

The point of JG05 was to be able to delete BundleHandler and all the associated complex logic ("Resolve ... bundle
key"). LayerBuilder.File.bundlevalue(String) already automatically detects fixed string vs. "#key" (assumed to be from
Bundle.properties in same package) vs. "some.Bundle#key", so normally processors need not do anything; in your case you
permit "" as a default value, so you should only need to check that, e.g.

String shortDescription = annotation.shortDescription();
if (shortDescription.length() > 0) {
    file.bundlevalue(Action.SHORT_DESCRIPTION, shortDescription);
}

(Verification of the existence of a bundle key is a nice idea but it should be in openide.filesystems, not here.)
Comment 24 Miloslav Metelka 2009-04-10 11:13:31 UTC
Oops, I've forgot to commit MnemonicsTest separately :(
I'm willing to do a general verification of bundle value in openide.filesystems but I would like to implement some caching of the parsed bundles so that the 
bundle does not have to be parsed with each invocation of file.bundlevalue() (currently I just use new 
PropertyResourceBundle(bundleFileObject.openInputStream()) ). Anyway the API change is integrated and this can be done later once I fix possible regressions 
of this integration.
My thanks to Yarda and Jesse.
Comment 25 Jaroslav Tulach 2009-04-10 14:50:20 UTC
Oleg, as soon as this gets propagated to main we shall compare loaded classes. There shall be about ~100 less loaded 
classes from editor. If true, we can shrink the whitelist.
Comment 26 Jesse Glick 2009-04-10 15:57:39 UTC
To JG05 - actually it seems bundlevalue(String,String) already checks the source Bundle.properties for the key. So your
annotation processor does not need to do anything.

(If caching is really necessary, a WeakHashMap<ProcessingEnv,Map<String,Properties>> would do the trick.)
Comment 27 Jesse Glick 2009-04-10 16:12:29 UTC
BTW for JG06,

if (LOG.isLoggable(Level.FINE)) {
    LOG.warning("Value of property \"" + propertyName +
            "\" of AlwaysEnabledAction is \"" +
            value + "\" but delegate has \"" + delegateValue +
            "\"\ndelegate:" + delegate + '\n');
}

can be simplified to

LOG.log(Level.FINE, "Value of property \"{0}\" of AlwaysEnabledAction is \"{1}\" but delegate {2} has \"{3}\"",
        new Object[] {propertyName, value, delegate, delegateValue});

since java.util.logging defers expansion of formats in log messages unless and until the message is actually printed.
Comment 28 Quality Engineering 2009-04-11 06:54:06 UTC
Integrated into 'main-golden', will be available in build *200904110201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/fa07f7c7f370
User: Miloslav Metelka <mmetelka@netbeans.org>
Log: #150875 - Too many editor Action classes loaded at startup/warmup.
Comment 29 Miloslav Metelka 2009-04-14 23:30:05 UTC
Implemented Jesse's suggestions - http://hg.netbeans.org/jet-main/rev/563ce480bba0
Comment 30 Stanislav Aubrecht 2009-04-16 08:18:21 UTC
*** Issue 162610 has been marked as a duplicate of this issue. ***


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