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.
The task is to provide a template that will not instantiate the DataObject when the file is visible in New From Template dialog but only when it is really instantiated.
Created attachment 7902 [details] Module that registers template in lazy way
Solution is based on usage of group module. The template itself is just a .group file containing just one reference to the real .lazy file. The presentation in NFT is provided by .group module, no lazy.MyDataObject is instantiated until user clicks NFT's Finish button. Tested with -verbose:class: -loads DataLoader on startup -loads DataObject on startup (to resolve loader methods) After NFT Finish it loads DataNode meaning that before that there was no instance of lazy.MyDataObject
I presume this is only useful e.g. for the Templates/Other/ folder, which would be expanded even when most of the file types there are not to be used. For e.g. Templates/Classes/ or Templates/GUIForms/, if you expand it probably you are going to use it, so doing something like this is a waste of effort (needless complexity). Could .shadow be used instead? If the impl was fixed to not create the original DataObject until it was requested. E.g. if the ShadowNode is given an explicit display name and icon, it does not yet need to delegate anything. A little simpler for the module writer; .group files seem unnatural here.
> For e.g. Templates/Classes/ or Templates/GUIForms/, if you > expand it probably you are going to use it false. I may expand the folder and collapse it again while searching for the right template
"I may expand the folder and collapse it again while searching for the right template" - so what? So a few classes get loaded. You don't do this after you have used the IDE a few times and know where the templates are. Then you don't pay. I don't think it matters if there is a minor amount of extra overhead for people who are still exploring the application, opening every menu, trying every dialog, etc. Of course that will load more stuff.
"Could .shadow be used instead?" there is a problem with DataShadow.getOriginal when the shadow is broken. Otherwise after a bit of mangling it could.
I tried Jarda's technique out on the Java module, changing the JApplet template to use a group file instead, commenting out the rest of its templates and clearing everything else out of the Templates dir in the sfs. Then I ran it with verbose:class. The good news: - It works - no furious classloading when the new menu comes up. The not so good news: - Every module in the standard distro would need to be converted to use this pattern - The "group-of-one" thing is a hack, and will make module code harder to understand - All templates appear with "(group)" included in their template name (easy to fix) - Substitution of name from wizard in template name appears broken; however, the group module's javadoc is not very helpful Jarda mentioned one alternative, which is to hack LoaderPoolNode.NbLoaderPool so that it will use a dummy loader for items under the Templates folder, which could also be an option. This would break editing of templates directly from the Templates folder in Options (not, I think, a commonly used feature), but would keep module code cleaner. Implications for platform users - ??
I'm wondering if there isn't a solution possible from a different approach (tell me if I'm crazy here). In this case, we have a folder where all of the display names, sorting criteria and icons can be explicitly assigned in XML. So all of the criteria to display it can be found from the FileObject representing a template - the DataObject isn't needed unless its contents or actions or properties or cookies are needed for something (they aren't in a menu, possibly also in other situations if untouched by the user). Couldn't DataFolder simply be smarter about this - i.e. CreateNodeDelegate creates child nodes that look for these properties on the FileObject, and create the DataObject only if something requires a property not specified available as an attribute? If this is too radical, what if this behavior could be turned on by some property of the folder specifiable in XML? It strikes me there may be other cases where you don't necessarily need a DataObject in order to represent a FileObject in the UI. <implementation detail> Possibly these child nodes would create a DataShadow that instantiates and delegates to the DataObject when asked for something it can't deliver from the FileObject's attributes... </implementation detail>
A folder cannot know for sure how its child nodes must be displayed. It can make some guesses, but they are just that - guesses. Any data object is permitted to produce any node delegate it wishes, which may look and behave however it wishes, and Tim's proposal would display them wrong. Yarda's shadow idea seems safer, if a little clumsy. I still don't think this is worth fixing except perhaps for the case of the Other folder...the laziness will only be of benefit to people who routinely open template folders and then close them again without using them, which is sort of dumb (and if it really happens a lot we should ask nbui for more descriptive folder names!). I think there are plenty of more serious use cases of the IDE which we know are annoyingly slow that should take priority.
Analysis of effect of module classloading when browsing the New menu on the explorer popup menu. Test application: S1S EE 4 update 1 Test machine: Ultra 60, 512Mb RAM, 400Mhz Methodology: Verbose classloading log analysis, visualgc for memory analysis RESULTS ------- Total seconds spent unresponsive (GC and classloading): 10.447 PermSize increase: 5.5M OldSize increase: 5.071 Seconds in GC: 4.993 Seconds loading classes: 5.454 Number of classes loaded: 1159 Folders producing heaviest classloading: Beans - 357 classes (loads pieces of the Java module which it depends on) EJB - 174 classes SOLUTIONS: GC time can be reduced or eliminated by improving GC settings (e.g. -J-XX:PermSize=32M - ensure enough room in permanent area for these classes) Use declarative syntax and group files or other solution such that DataObjects are not created for the contents of the Templates folder. OR Reduce classloading required to create a DataObject for some kinds of file DATA ---- BEFORE browsing all full contents of templates menu: PermSize: 18M OldSize: 27.695M EdenSize: 3.438M GC Time: 7.938 Old GC: 5, 6.058s Classloader time: 4345 loaded, 87 unloaded, 6.313s AFTER browsing all full contents of templates menu: PermSize: 23.500M OldSize: 32.766M EdenSize: 4.062M SurvSize: 64K GC Time: 12.301 Old GC: 6, 9.168s Classloader time: 11.767s, 5504 loaded, 92 unloaded, 11.767s TO REPRODUCE DATA: Log file search key for START of menu browsing: [Loaded org.netbeans.core.windows.WindowManagerImpl$TopComponentManager$DoRequest] [Loaded sun.reflect.GeneratedMethodAccessor35] [Loaded sun.reflect.GeneratedMethodAccessor36] Log file key for FINAL entries during menu browsing: [Loaded org.netbeans.tax.TreeNotationDecl] [Loaded org.openide.actions.EditAction] Oddities: Why would creating a Java DataObject drag things like: org.netbeans.editor.ext.java.JavaLayerTokenContext org.netbeans.editor.ext.html.HTMLSettingsInitializer$HTMLTokenColoringInitializer into memory?
Created attachment 7956 [details] Classloading log for period while browsing all submenus of the New context menu
Re Jesse's comment: While a folder by definition cannot know how to display all items it contains, there are cases where folders used for known purposes *do* also contain all relevant data needed to display their contents. With the (significant?) exception of cases where someone has added customized templates, this menu is one such instance; I expect there are others. NetBeans uses files heavily to represent objects other than user-editable data, yet all are presented using a common framework (DataObjects) that assumes that any such object may need to be edited, and necessitates loading logic for all possible user operations on these files. In the case of templates, this is not (under normal usage) user-editable data, but it is treated like it is. The notable exception in the case of templates is when a user might have added new templates to an existing folder. In that case, that data exists as files on disk, without the constraints defined in XML that exist for templates declared by modules in XML. If the supported and documented way to add custom templates to the existing templates is to create a new subfolder of Templates, this is a non-problem - classloading happens on viewing the subitems of a folder. Is not being able to flag such folders as containing unlikely-to-be edited data a reasonable solution?
"there are cases where folders used for known purposes *do* also contain all relevant data needed to display their contents" - not when template folders can be added to by anyone, user or 3rd-party module. You are gambling if you assume you know what is in them. Maybe it's a good compromise if the benefits are overwhelming, but don't pretend that the code will always be correct. Re. templates not being editable/extendable by users: ??? Sure they are. It is quite normal and supported (in GUI and documentation) for users to add their own templates, of any type, to any folder, rename templates, edit them, etc.
A concern with any solution that defers creating the DataObject: This will work in the context menu (which we plan to delete soon). I am afraid it will not work so well in the wizard: The template wizard shows a description for the template. A template's description is not assigned in the XML layer declaring the template (could it be?). This means that there will be no classloading/gc penalty for opening a folder. But as soon as the user selects a template, all of the loading needs to happen. So we will have shifted the pause until a template is selected, but the pause will still be there. Acceptable? Unacceptable?
Opening a folder of templates *is* a responsiveness problem in the New Wizard. For example, expanding "Java Classes" folder for the first time takes more than 2 seconds (733MHz, W2K). It really does not look good. The user sees no apparent reason why it takes so long to show several items in the tree. About 500 classes are loaded in that time (similar to expanding an ordinary folder with java files). Jesse is right that the classes will be loaded sooner or later anyway, so just avoiding that in the new wizard (menu) is not a solution - the time appears somewhere later, slowing down something else. Maybe a candidate for a "warmup" task (see issue 28596).
Yes, the classes would be loaded anyway, sooner or later - but it would be better not to load them at exactly the time the user is waiting for something to happen. It could be a candidate for warm-up. In the case of the Java module, would it be possible to reduce some of the aggressive classloading? For example, opening the Beans folder means that things like org.netbeans.editor.ext.html.HTMLSettingsInitializer$HTMLTokenColoringInitializer are loaded. That can't really be needed to simply display a Node for a DataObject.
Hmm.. I would really like to learn how to avoid loading of parser classes when the IDE - in this N.F.T. Wizard - attempts to use the parser for the first time. I can eliminate the thing by disabling a check for "runnable" class for Templates (icon can be given in the layer), which will hopefully postpone parsing to the second step of the Wizard. At that point it is almost unavoidable to load all those classes, since the Wizard works with the source's (parsed) structure. I once thought about storing pre-parsed template's structure (and possibly contents of template's superclasses for Override methods panel to work - yuck) somehow into the layer - that would obviously not work for user supplied templates and postpone the loading only to the final phase of the Wizard, when it generates the code. The result will be fast browsing through templates and a responsiveness issue when the user presses "Next >" button.
vhen .group files used for templates a piece of public API should be provided
Created attachment 8050 [details] Patch to the group module to turn of link indication and allow localized display names for child files
The above patch allows template files to specify that they should not show linking information in their templates, and allows names of children of group files to be localized. This gets rid of "(group)" in group-of-one templates and allows children of group files to specify their own display names (so the user edits Options | Templates | JApplet | JApplet template" not "... | __JApplet__.java"
Note related issues 29002, 29004 & 29005 - if we do *not* choose to use the group file fix, then this issue depends on them. If we *do* use group files, they are not as relevant to this issue.
Tim, you do not need to patch the group module to support special naming. For that systemFS.localizingBundle and Bundle.properties with File/Name.ext=Your Name is enough
To Svata: ideally if the template had a SFS.icon, then JavaNode.getIcon would only use this, and never have to use the icon base. However DataNode.getIcon simply gets the icon and then passes it to the FS for annotation. This is unfortunate because there is no way to predict if the FS is actually going to *use* the bare icon or not; SFS does not (if you specify SFS.icon), but e.g. LocalFileSystem leaves it unchanged, and VCS FS's use it (badged). I suppose JavaNode could pass a special Image to the file system which is actually a proxy: if any methods are called on it, it would call some method getNormalIcon and proxy to it, otherwise it would not. This would avoid doing complex calculations as for the executable badge unless they were actually required.
Created attachment 8367 [details] Patch to DataShadow to allow lazy DO instantiation
Created attachment 8368 [details] Demo patch to the layer file for the Java module demonstrating conversion to lazy loading
I've just attached a patch to allow lazy instantiation of DataObjects underlying ShadowNodes. Given the discussion on nbdev, it sounds like this patch will not be terribly useful outside the context of templates. Per that discussion, DataLoaderPool.ShadowLoader will only use the lazy constructor in the case that a shadow file has the attribute "isLazy=true." This will preserve the current behavior for the rest of the system. Also included (for MenuView - current discussion on nbui suggests that there will be some templates on the context menu after all) is the attribute SystemFileSystem.helpID (apparently *somebody* already uses this - I found mention of it in another issue). I know Jesse requested that no references to "SystemFileSystem.*" be included in openide. This does not look possible - getting the help context, etc. will force classloading. One option is to simply provide a different set of attributes, e.g. "Shadow.icon" or whatever. Please review.
Re. help context - just drop it. No one really cares that much about the helpID on the context menu item, it is not worth preserving I think.
Created attachment 8537 [details] A better patch (the earlier one corrupted a few options display names). No longer at all alters the default behavior of non-lazy DataShadows.
reassigning to Tim as he's the driver of getting impl done. Right?
Misc. comments re. the last attached patch. Re. NewTemplateAction: I don't understand this code (and similar elsewhere in the class): DataObject obj = (DataObject) n.getCookie(DataShadow.class); if (obj == null) obj = (DataObject) n.getCookie(DataObject.class); Shouldn't ShadowNode.getCookie(DataObject.class) give the DataShadow to begin with? Generally (and there is a unit test written for this) all DataObject's should satisfy: d.getNodeDelegate().getCookie(DataObject.class) == d Is it necessary for the new DataShadow(FileObject,DataLoader) constructor and getOriginalPrimaryFile() to be public? Are they called from outside org.openide.loaders.*? getOriginalPrimaryFile Javadoc: s/DataShadow it points to/DataObject it points to/ getOriginal & resolveOriginal: IMHO if resolveOriginal fails with an IOException, getOriginal should throw some sort of runtime exception immediately (IllegalStateException?) rather than return an undocumented null, which is likely to just cause NPEs later anyway. isTemplateImpl should be package-private I guess. Object o = (Boolean) DOPrimary.getAttribute(PROP_TEMPLATE); return (o != null) && Boolean.TRUE.equals (o); => [more simply & robustly in case !instanceof Boolean] return Boolean.TRUE.equals(DOPrimary.getAttribute(PROP_TEMPLATE)); NbBundle.getBundle(bundlename) can throw MRE, please catch & handle it. It will *not* return null - check the Javadoc. return rb.getString (fo.toString()); => [there was a reason I tried to deprecate this :-)] return rb.getString(fo.getPath()); ErrorManager.getDefault().log (ErrorManager.INFORMATIONAL, "Missing display name key for " + fo + " in resource bundle " + bundlename); //NOI18N Note that // NOI18N only applies to the line, not statement, it appears on. Also I think you want ErrorManager.WARNING here, not .INFORMATIONAL which will almost surely never be printed (non-qualified EM instance should never be used for logging at INFORMATIONAL level). // if (!getOriginalPrimaryFile().getFileSystem().equals ( // Repository.getDefault().getDefaultFileSystem())) return null; This check should not be commented-out, I think; SystemFileSystem.* attrs are only meaningful on the SFS. ds.getPrimaryFile().getFileSystem().getStatus().annotateIcon (i, type, ds.files()); What is this line for? (1) The return value is ignored! (2) Who cares about calling SFS.status.annotateIcon anyway? You already have the desired icon - i. ProxyShadowNode.getDisplayName calls displayNameForShadow w/o checking for null return. And when will name.length() == 0 anyway? And why does it bother calling the original annotateName when it already has a display name from displayNameForShadow? getOpenedIcon(int) should delegate to n.getOpenedIcon, not n.getIcon. getDeclaredIcon(int type) ignores type param. Color vs. B&W is usually ignored anyway, but "SystemFileSystem.icon32" is a defined file attr. "//code below should theoretically work but doesn't." - please improve the comment or fix the code before committing. :-) Re. comment in getShortDescription - we should just display tool tips in the New wizard. There is already an HTML template description which is more useful. getActions, getContextActions, getDefaultAction - pls. update to delegate to new javax.swing.Action methods. Re. getHelpCtx - new HelpCtx(URL) is deprecated, and SystemFileSystem.helpID may only be a String. Why does getPropertySets not call die().getPropertySets() like other surrounding methods? setShortDescription and setDisplayName should simply throw runtime exceptions; they should not be called. ProxyDOChildren.addNotify: if isLeaf == null, shouldn't this also delegate to getOriginal().getNodeDelegate() to be safe? Or is it intentional that the default is effectively true? Also should PDOC.addNotify attach a listener to the original children, as does FilterNode.Children? Should TW.getDescription check for templateWizardURL on the .shadow as well as the original file? Ditto getIterator()? Minor comment: please avoid gratuitous formatting changes, like dumping spaces at the end of a line (e.g. DataObject.java diff). Makes patches harder to read, kills cvs annotate, etc.
BTW when you have a patch for something, that definitely means STARTED...
Thanks, Jesse, this is exactly the kind of review I was looking for. I'll make the changes you suggest.
Jesse, I've integrated your comments and will attach a revised patch. >Re. comment in getShortDescription - we should just display tool tips >in the New wizard. There is already an HTML template description >which is more useful. I assume you meant "just not display tool tips"? If so, I agree. However, when you're fetching a tooltip (which will turn out to be empty) is one time when you know the user isn't waiting for something else to happen, so it's actually a pretty good time to trigger classloading of the module in question - it means less of a classloading-induced delay when the user clicks the next button. > Also should PDOC.addNotify attach a listener to the original > children, as does FilterNode.Children? In practice, the only place this can become relevant is if you edit a template by opening its context menu in Options. However, the method copyNode in org.netbeans.core.projects.SettingChildren forces instantiation of the backing DataObject, so you'll never actually see a ProxyShadowNode in the options tree. Theoretically that could change, but it's pretty doubtful it will change in a way that won't force instantiation of the DO. I can add the code if it seems necessary, but in practice PDOC only exists to defer fetching the children object (and thus needing the DataObject and its delegate) in the constructor of ProxyShadowNode. Of course, that means I could take it further and just use Children.LEAF, but that seems like straying too far from the behavior of the real ShadowNode. > getActions, getContextActions, getDefaultAction - pls. update to > delegate to new javax.swing.Action methods. No way I can see to do it without die() getting called twice (if I override getActions, I need to call die() in getActions, and getActions will call either the old getActions() or getContextActions(), which also need the ShadowNode returned by die(). Cleaner to override the two deprecated methods so calls to the new getActions or them will work properly. Or did you mean something else?
Created attachment 8586 [details] Patch revised per Jesse's comments
Hmm, I have some concerns about the benefit of the lazy templates - at least for "java" templates. I've tried the patch (the previous one, not that very last), but it seems to be even less responsive than the current solution. As I tried it, expanding Java Classes template folder was slightly faster (but not much, like from 1300ms to 1000ms; anyway about half of classes loaded - good), but the time to process "Next" is substantially longer (cca from 2s to 4s). The pass-through responsiveness of the New wizard is then worse, not better. The reason is probably in that the template dataobject must be parsed for the second step - this was normaly done in background after expanding the templates folder node (i.e. finished in the time user is catching up, selecting the template and clicking Next). While with the patch, this all starts after pressing Next, so blocking... I think the "lazy templates" idea is generally good one, but should be consistently implemented over all wizard steps - i.e. not loading/creating any module stuff until some objects really starts to be created (after clicking Finish). But that's hardly possible now. The original reason for thinking about the "lazy templates" was IMO to improve the New... context menu for browsing - and here it would certainly help. But the menu was removed... Now I personally believe that "warm-up" is better solution for java templates here (but I am probably already bothersome with the warm-up ;). Opinions? Am I wrong? (Please note my objection is mainly to java templates, it could work quite good for the other, so I don't want to just cancel this.)
Yes, the classloading happens when you click the Next button - there's no way to avoid this (and managing it in a wizard is extremely unlikely - any request for a cookie other than DataObject from a template will cause the DataObject to be instantiated. The good news is you don't load classes from other modules which you are only browsing the templates. So if I browse the RMI template, I don't load the RMI module's classes unless I click Next.
Created howto for module authors: http://performance.netbeans.org/howto/lazy-templates.html
Removing issue 28981 ("add Group API to provide contained links") from dependencies - proposed solution no longer involves group files.
Uh, but you forgot to actually remove #28981. :-)
I don't see it in the dependency tree anymore. ???
OK, I've tried the latest patch and it works better for me - the 1s delay (that worried me) to expand Java Classes folder for the first time went away, now it is around the ideal 100ms. So browsing through templates can really be lightweight and fast. But on the other hand, the next step is really slow now - takes almost 5 seconds (normally 2s) for the first java template (tried JApplet and Main). Also the time to finish the wizard and open the new java class is longer (5.5s vs 4.5s). But this should be probably solved as another issue...
Yes, nothing you can do about the delay when you press Next - the rest of the wizard really needs the DataObject (really it ought to work with some kind of model, IMHO - but then we're just moving the work to the end of the wizard)... We could put some hack into the wizard to display the wait cursor, I suppose...
Do we plan to implement this for S1S4.2? It might be usefull IMO. BTW: context diffs are friendlier
FYI, I am doing some changes to make the patches work with the new (reborn) New context menu, which had been removed when this work was started, and this will be committed to the trunk soon. WRT the New context menu: I may have to change the triggering mechanism for using lazy DataShadows from looking for a special property on the file "isLazy" to checking for "isTemplate." The reason: Otherwise, all templates in the pop-up menu which - are shadows - have not been updated to be lazy will show the (->) link symbol. The code for these nodes is in org.openide.actions, which knows nothing about lazy templates (all the lazy stuff is loaders package-private). The implications: Just one - if you create a link in your project to a template in Tools | Options | Templates, it will NOT show the (->) text Does anyone see a problem with this?
Good news and bad news with using this patch with the revived new context menu. The good news: - It works and the data objects are not created The bad news: - All the classloading for the Java module happens between the time you click the menu item and the time the wizard appears. This makes a LONG delay in showing the wizard. I'm working on getting the wait cursor happening for this - I don't see any other solution (short of rewriting the Java module's wizard to use some kind of lightweight data model instead of creating java data objects). The alternative is to kill the "Priviliged" items on the context menu and have only recently used ones (for which the classloading has already been done).
I will postpone integrating this until early next week, so there's time for discussion. To reiterate, we have the following options: - Do integrate lazy templates and live with a long pause on first invocation from the menu - Do integrate and try a warm-up task for JavaDataObject - Do integrate and kill the new "Priviliged" list of templates on the submenu. Then first invocation will always bring the wizard up on the templates page, quickly (the classloading still happens when they press next) - Svata finds some magic way to reduce the classloading hit for creating the first JDO in the system, or get the wizard not to do it - Don't integrate lazy templates
FWIW my preferences would probably be: - don't do anything special for templates in folders which have not been expanded; if users expand them, and some class loading starts, tough luck - "well don't do that then" - put pressure on module authors not to do too much class loading just because a DataObject/DataNode is constructed - making a DO should be very lightweight anyway, for other reasons - make sure you can click Finish on a JavaDataObject template after setting the name without *ever* loading all those stupid extra panels that let you add constructors etc.; people who do not use this stuff should not pay for it - use a warmup task as needed to preinit some important template folders, or at least the privileged templates, so any loading delays are paid asynch while the user is not waiting - otherwise keep impl *simple* and avoid any kind of weird tricks that would be hard to explain to module developers (.shadow's, magic file attributes, etc. still make me nervous here)
New facts: - goal of this issue (speed up New and Mount dialogs) is now mostly obsoleted because of java warmup patch, which helps significantly. - still it would be good to have lightweight templates, but it's not strongly important for Tegal 3.5 release - we won't try to integrate into Tegal, so removing milestone keyword.
Created attachment 9116 [details] Final archival copy of lazy template patches (w/ fixes for updated popup menu)
Closing this issue - warmup tasks and coming datasystems replacement make this a non-issue.
verifying