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.
Summary: | Enabling/disabling module takes an incredible amount of time | ||
---|---|---|---|
Product: | platform | Reporter: | Jiri Skrivanek <jskrivanek> |
Component: | Module System | Assignee: | Jesse Glick <jglick> |
Status: | VERIFIED FIXED | ||
Severity: | blocker | CC: | mmirilovic, pnejedly, rmatous |
Priority: | P2 | Keywords: | PERFORMANCE |
Version: | 3.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 20628 | ||
Attachments: |
Full thread-dumps
Possible patch File events fired while disabling properties module w/o BinaryFS patch Proposed additional patch; removes special hashCode behavior, now just hashes by path |
Description
Jiri Skrivanek
2002-12-06 13:36:23 UTC
It's true, I have made 2 thread-dumps, first is after 5-10 seconds and the second one after 45-50 seconds, disabling AutoUpdate module too. (see attachment) Created attachment 8192 [details]
Full thread-dumps
Jesse, it rises after your changes in binary XML caching. Or maybe somethig else causes this one ? True, it does seem slower. Probably because with the binary cache manager, the cache layer must be replaced rather than updated in-place. I could interpose a MultiFileSystem with one delegate; not clear if that would help. Probably the root performance issue is gratuitous event firing from MultiFileSystem, which only Radek would be able to solve. With the XML cache manager, the XMLFileSystem is updated in-place, which apparently fires few events. But MFS seems to behave very differently. Applies also to first (cacheless) startup, though not as extreme. I don't think there's an easy solution here. The problem is not in MultiFileSystem - which seems to fire only necessary changes - and recreation of the disk cache is reasonably fast. The problem is in the filesystem itself and the replacing of it. Try just turning off the image module - which only has a few actions in Actions/View/, a folder probably never even traversed at that point. This locks up NB for a half a minute or so of 100% CPU. Thread dumps show that the entire SFS is getting changes fired, triggering remounting of filesystems, recreation of the whole menu bar, recreation of lookup, etc. The problem is that almost all of the SFS is in the cache. The ModuleLayeredFileSystem when recreating the cache calls setDelegates with a new, almost-identical BinaryFS. It sees that all delegate FileObject's have changed, and fires changes in everything. Really most of the new FileObject's are exact clones of the original ones, but MultiFileSystem does not know that. Possible solutions: 1. Make BinaryFS's FileObject.equals/hashCode correspond to file contents. Maybe MultiFileObject/AbstractFolder will respect that and treat the file as unchanged. This would be ideal. It would just need to keep a CRC of file name, contents, and attributes, I guess. 2. Make BinaryFS extend AbstractFileSystem and do an in-place load. AFS then takes care of figuring out what changed. More obvious implementation, but may introduce a lot of memory overhead - the current BinaryFS is quite streamlined, while AbstractFileSystem is more general and may have a higher per-file memory consumption. Created attachment 8330 [details]
Possible patch
I have a patch using approach #1 which seems to work fine. Disabling or reenabling a module is fast again (I did not check first startup) and seems to work: the expected layer-supplied things are turned on or off. I would like to get a review from both Radek and Petr though. Especially from Radek - I cannot really understand MultiFileObject and AbstractFolder from the source code, it is too complicated. I do not know whether the pre-patch overhead was from firing file add/remove events, or just attribute changed events. Created attachment 8331 [details]
File events fired while disabling properties module w/o BinaryFS patch
Attaching events fired to >=1 listener without the BinaryFS patch (though with the MultiFileObject patch, in case it matters). As you can see, both file attribute changes and file changes are fired for apparently everything in the system file system. With the BinaryFS patch, there is only one change fired: Dispatching file changed: Modules/org-netbeans-modules-properties.xml and I presume Templates/Other/properties.properties would be fired too if someone were listening to it - in fact if you open the Other templates category in Options first, the template does disappear while the module is being disabled. I now also have a unit test confirming that removing a layer from the binary cache does not fire gratuitous changes (and this test passes only with the patch applied to both MultiFileObject and BinaryFS). Optimistically committing fix. Would still like a review - the patch (just code, not test) could be rolled back if it looks like trouble. committed * Up-To-Date 1.4 core/src/org/netbeans/core/projects/cache/BinaryFS.java committed * Up-To-Date 1.2 core/test/unit/src/org/netbeans/core/projects/cache/BinaryCacheManagerTest.java committed * Up-To-Date 1.3 core/test/unit/src/org/netbeans/core/projects/cache/CacheManagerTestBaseHid.java committed * Up-To-Date 1.3 core/test/unit/src/org/netbeans/core/projects/cache/data/layer1.xml committed * Up-To-Date 1.108 openide/src/org/openide/filesystems/MultiFileObject.java added * Up-To-Date 1.1 openide/test/unit/src/org/openide/filesystems/MultiFileSystemRefreshTest.java I can't speak for MFS, only Radek can assure you it is correct to use equals() instead of == For BinaryFS, the patch looks OK although it seems too complicated to me. If you compare the behaviour with the XMLFileSystem.setXmlUrls(), you'll realize that the XMLFileSystem is effectively unable to recognize any change in the content of a file (it can only report different date, which may be enough) Now, what scares me is the recursive check in the folder implementation. That means that anybody who will get hold of root FO (I hope only MFS will) and calls equals() on it will force full fetch of the FS (Oh, BFS it is written well enough to fetch only folders in that case). Why should it do a recursive check? If I have folderA->folderB->fileC and fileC disappears, only folderB should fire event, not folderA, right? "For BinaryFS, the patch looks OK although it seems too complicated to me. If you compare the behaviour with the XMLFileSystem.setXmlUrls(), you'll realize that the XMLFileSystem is effectively unable to recognize any change in the content of a file (it can only report different date, which may be enough)" - honestly I have no idea how XMLFileSystem works; I could not understand it from reading it. I just knew I had to somehow differentiate a real change from no change, and BinaryFS has no notion of last modified time for a file object beyond cache load time. Consider the case where you reload a module where the only change is in the content of some layer file. (Note: test modules do not get their layers cached. But you can reload modules not in test mode, sometimes.) "Now, what scares me is the recursive check in the folder implementation. That means that anybody who will get hold of root FO (I hope only MFS will) and calls equals() on it will force full fetch of the FS." - not with a tiny additional patch to BFSBase.equals to compare the paths before hash codes. Then the only time that a recursive check would be done during equals is if you have two different folders with the same path - which only happens during a cache reinvalidation, which is unusual and needs to be correct. However hashCode might be called from MultiFileObject, so I will remove the recursion in BFSFolder.hashCode - it is unnecessary anyway. If two folders are equal, they must share the same path, so just hashing the path is enough. Ditto BFSFile - so specificHashCode is actually unnecessary, and I will just delete it. "Why should it do a recursive check? If I have folderA->folderB->fileC and fileC disappears, only folderB should fire event, not folderA, right?" - right (though listeners on folderA will also receive the event: the source should be folderB). However without doing a recursive check, the old folderB and the new folderB would be equal! (Same path.) I think MultiFileObject refreshes the children anyway, which would fire a change event, but I did not know exactly how MFO was implemented and wanted to be safe: for other filesystems, the old and new folder would be different (!= and !equals), so I wanted to maintain that semantics *except* in the case that the new folder was really interchangeable with the old in all respects: same path, same children, same listeners (none, since BinaryFS is r/o). Created attachment 8348 [details]
Proposed additional patch; removes special hashCode behavior, now just hashes by path
"However without doing a recursive check, the old folderB and the new folderB would be equal! (Same path.)" No. I was thinking about two folders being equals if they have the same path *and* the same names of the children (only keyset, not the values - FOs). Anyway, it is OK now - hashCode is cheap, equal is cheap for: a) different type b) the same instance c) different path and there is no way to fail all of the above except when switching layers. Good work! "I was thinking about two folders being equals if they have the same path *and* the same names of the children (only keyset, not the values - FOs)." - my concern would still be that some filesystems code would see the old and new folder, think they are identical, and not refresh children for that reason. I'm not sure if that would actually be the case or not - only Radek would know, I guess. I am trying to be conservative since there is no documented semantics for FileObject.equals that I am aware of. So yes, I hope that equals will be cheap when it should be cheap with the refined patch - I will try to commit it as soon as I get a chance. Thanks for review help Petr, I forgot that someone might call equals() or hashCode() on the root BFSFolder at some random time! :-( verified in [nb_dev](20021218) Removed tricky hashCode behavior: committed Up-To-Date 1.5 core/src/org/netbeans/core/projects/cache/BinaryFS.java |