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.
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.
Created attachment 72393 [details] grep "Action$" all-loaded-classes.txt
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
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?
> 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.
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?
Created attachment 78077 [details] method to refactor
Created attachment 78078 [details] Method after control r
Created attachment 78079 [details] Method with characters to delete highlighted
Created attachment 78080 [details] Method after command x
Created attachment 78081 [details] Method with characters to replace highlighted
Created attachment 78082 [details] Result after command v
Something got messed up because the attachments were suppose to go to 150139.
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.
Created attachment 79057 [details] Diff of the @EditorActionRegistration related changes
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!
Can we consider this patch for 6.7, please?
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).
Created attachment 79669 [details] Patch file after Yarda's suggestions
[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.
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.
Created attachment 79793 [details] Patch #3
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.)
http://mmetelka@hg.netbeans.org/jet-main/rev/fa07f7c7f370
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.
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.
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.)
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.
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.
Implemented Jesse's suggestions - http://hg.netbeans.org/jet-main/rev/563ce480bba0
*** Issue 162610 has been marked as a duplicate of this issue. ***