Bug 29354

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