Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebPathField Performance #698

Open
tullisar opened this issue Apr 27, 2022 · 8 comments
Open

WebPathField Performance #698

tullisar opened this issue Apr 27, 2022 · 8 comments
Assignees
Projects

Comments

@tullisar
Copy link

I noticed that when resizing a frame that contains a WebPathField, the updatePath method gets called quite a lot more than seems necessary. I dug a little, and it seems as though the resize listener for the text field part of the WebPathField does the following check:

if ( !pathField.isShowing () )
{
    updatePath ();
}

The isShowing() method always returns false since the pathField does not have a component peer set it seems. I can reproduce this with the following simple test (assuming running in the event dispatch thread):

WebLookAndFeel.install();
JFrame frame = new JFrame("Test");
WebPanel panel = new WebPanel(new BorderLayout());
frame.setContentPane(panel);
WebPathField field = new WebPathField();
panel.add(field, BorderLayout.CENTER);
frame.pack();
frame.setVisible(true);

updatePath can sometimes be a bottleneck, when file filters and disk/network I/O are taken into account. Would it be better to have that listener check that the WebPathField component itself is showing rather than the text field child component?

@mgarin mgarin self-assigned this May 11, 2022
@mgarin mgarin added this to Planned in v1.3.0 via automation May 11, 2022
@mgarin
Copy link
Owner

mgarin commented May 11, 2022

Yes, unfortunately the current WebPathField implementation has significant performance issues whenever it is getting resized since it updates it fully updates its content whenever size changes, even if it isn't needed at all.

Path elements creation and update simply needs an overhaul:

  1. All path components should be created upon initialization and kept until actual path changes
  2. Any path components that don't fit within visible container area should be hidden via layout and/or visibility
  3. Whenever size changes but actual visible path components do not change - there should be no actions taken
  4. Whenever size changes cause visibility to change for some of the path elements - simply update those
  5. Whenever path changes - all (or some) path components should be recreated

I was planning to update WebPathField along with some major changes to file chooser, but it does make sense to fix this issue separately as well, so I'll see if I can fit it in one of the smaller updates.

And sorry for a late response, I missed the notification for this issue.

@tullisar
Copy link
Author

Since a file chooser dialog (either created with JFileChooser or WebFileChooser) uses a path field, just constructing and displaying a file dialog also results in a lot of redundant/excessive directory listings. Using any file chooser dialog on our internal network is achingly slow, and even displaying the chooser dialog itself takes several seconds. We have some endpoint and network intrusion software running internally that I suspect is slowing the listing of network directories. The base JFileChooser without WebLAF installed does not seem to have performance issues however.

Is there a way to force a non-WebLAF file chooser dialog to be used while the rest of the application uses WebLAF? I've temporarily replaced my path field use with text fields as well, but I can't avoid the file chooser dialogs unfortunately.

@mgarin
Copy link
Owner

mgarin commented Jun 15, 2022

I'm afraid it's not that simple to switch JFileChooser to any other implementation as they're all usually tied to the L&F and depend on all the corresponding properties provided by that L&F.

I've looked into current WebFileChooserUI and WebFileChooserPanel implementations and the long loading time is most probably coming from the path field and file tree loading all possible system roots via File.listRoots() and checking whether each of them is a directory - I get similar delays with that call in my office network with multiple network drives attached.

The problem here is that there is no good way (as far as I am aware) to avoid "poking" the network drives while trying to retrieve drives list on the old Java IO API - it is synchronous (done via FileSystem.listRoots() in a system-specific implementation) and it is substantially slower than NIO alternatives.

In NIO there is a FileStore type which can be used to detect the drive type for instance, and iterating through drives is faster and can be done asynchronously which is also a huge benefit for the UI.

Unfortunately I can't use NIO yet because project is still supporting JDK 6u30+. I also don't think I can do much about synchronous roots loading because the current file tree and path field implementations do not support asynchronous roots loading and it would require far too many changes in many parts of those components to allow that. Tree does support asynchronous loading of any of the children, but that doesn't help much as that doesn't change the initial file chooser load time.

I only see a couple of things that can be done to optimize file chooser load time at the current stage of the project:

  • Create a unified model for the file chooser that would handle loading & caching of all files for tree, path field and list to avoid them doing that separately and multiplying the load time

  • Optimize WebPathField performance by reducing the amount of updates and changing the way they're done

  • Make all the initial model data loading asynchronous to avoid completely locking out the UI, this can potentially be done by simply hiding all the file components and showing some sort of loader instead during the disks loading - this isn't the best solution, but it would work as a temporary fix

These might potentially help bringing file chooser load time more in line with the one from other L&Fs.

I'll have some time to look into that on these weekends, but I can't promise anything as this might involve quite a lot of changes across multiple components, not just the file chooser. But at the very least I can probably optimize path field and maybe add some caching for the roots loading as a temporary "dirty" performance fix.

@tullisar
Copy link
Author

That's understandable given the Java 6 support need. I was able to work around it a little bit with a dirty hack for now too. Before installing WebLAF I create a singleton instance of a JFileChooser class who's setUI method does not allow the UI to be changed after being set. I'm able to display and hide it without the user being aware so that all the UI resources are loaded. It's not ideal but it'll help in the mean time. I definitely understand the potential for this to not be an easy fix, but I do appreciate you looking into it!

I wonder if it might be useful if WebLAF allowed a custom FileSystemView/FileView/FileSystem to be passed for use with path fields and file choosers. This way one could write a decorator over the defaults that could either make use of NIO features or to skip network drives if desired.

@mgarin
Copy link
Owner

mgarin commented Jun 16, 2022

I wonder if it might be useful if WebLAF allowed a custom FileSystemView/FileView/FileSystem to be passed for use with path fields and file choosers. This way one could write a decorator over the defaults that could either make use of NIO features or to skip network drives if desired.

This is a very good idea, although I'm not sure if FileSystemView and FileView have enough options in their API to provide everything I need for the chooser. And FileSystem is a NIO class introduced in Java 7, so I can't really use that.

But there could be an alternative custom API made specifically for the chooser that has everything it needs with default implementation using old IO and potential to make a custom NIO one if you're using newer Java version. Maybe even not just for the chooser, but any file-related component in WebLaF.

The main issue - it still requires substantial changes for all involved components, but it is definitely something that could work really well even in the current project stage. I'll see what can be done in reasonable time.

@mgarin
Copy link
Owner

mgarin commented Jun 21, 2022

I've looked into potential improvements these weekends - unfortunately all changes that can be done and that I've mentioned are pretty significant. I've started with path field rework as it is potentially the biggest "offender" right now and finished approximately half of it. I'll push those changes into main repo once completed and a snapshot version will be available (most probably at the end of next weekends).

Once path field is revamped - I will be able to replace the old chooser panel with new one based on a unified model acting as data source for all the separate elements. Not yet sure if the tree will need some major updates or not, will see that when I'll get to it.

@tullisar
Copy link
Author

tullisar commented Jun 21, 2022

Along the topic of the decorator / alternate roots supplier option, perhaps WebPathField could be optionally supplied with a Supplier<File[]> (the version included in WebLAF) for listing roots in lieu of calling the FileUtils variants. Maybe something like:

public class WebPathField extends WebPanel
{
    // ... omitted things not relevant to this sugested
    Supplier<File[]> fileRootSupplier = null;

    // This function would be used where applicable in the class instead of calling FileUtils.getDiskRoots directly. Additionally, this allows the function to be overridden in a subclass. It could be assigned in a constructor or possibly changed with a set function too?
    protected File[] getDiskRoots()
    {
        return (fileRootSupplier == null) ? FileUtils.getDiskRoots() : fileRootSupplier.get());
    }
}

@mgarin
Copy link
Owner

mgarin commented Jul 1, 2022

It shouldn't be necessary (root supplier) when WebPathField will have its own model providing various methods for file system access (including roots). A bit more in-depth about it below.

I did some more work on the WebPathField on the past weekends - specifically I gave up on using the standard FileSystemView as it doesn't solve some of the issues I've mentioned before and would have to be modified anyway, instead I'll be using a custom model that has all the methods necessary for WebPathField and will have baked-in caching for some parts in default implementation.

I'm not yet sure whether this will be some kind of unified model between file chooser, path field and maybe some other file system -related components (similar to FileSystemView) or not (completely separate models), or maybe a mix of both. That is something I'll have to decide down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v1.3.0
  
Planned
Development

No branches or pull requests

2 participants