Community
Participate
Working Groups
The org.eclipse.ui.internal.Workbench class has a method: private final static void declareImage(String key, String path, boolean shared) { URL url = BundleUtility.find(PlatformUI.PLUGIN_ID, path); This is used to load images in a static initialiser with many different arguments: private final static void declareImages() { // Overlays declareImage(ISharedImages.IMG_DEC_FIELD_ERROR, PATH_OVERLAY + "error_ovr.png", true); //$NON-NLS-1$ declareImage(ISharedImages.IMG_DEC_FIELD_WARNING, PATH_OVERLAY + "warning_ovr.png", true); //$NON-NLS-1$ ... It's called from the workbench startup: Workbench::init method: activityHelper = ActivityPersistanceHelper.getInstance(); StartupThreading.runWithoutExceptions(new StartupRunnable() { @Override public void runWithException() { WorkbenchImages.getImageRegistry(); } }); This causes a lot of file traffic for all images even when they're not necessary.
The reason why it does this, by the way, is because it delegates to a 'find' method that attempts to do filesystem resolution by name (specifically, the FindSupport::find method) // Worry about variable substitution IPath rest = path.removeFirstSegments(1); if (first.equalsIgnoreCase("$nl$")) //$NON-NLS-1$ return findNL(b, rest, override, multiple); if (first.equalsIgnoreCase("$os$")) //$NON-NLS-1$ return findOS(b, rest, override, multiple); if (first.equalsIgnoreCase("$ws$")) //$NON-NLS-1$ return findWS(b, rest, override, multiple); if (first.equalsIgnoreCase("$files$")) //$NON-NLS-1$ return null; The net result is that a given file does a lot of lookups to determine if the files are there. Admittedly this is likely to use the list of a JAR file which may be cached but most of the time it's not necessary. Note that the result of this is an ImageDescriptor which is a pointer to the file anyway; it doesn't load any data until the getData is called anyway, which means the metadata lookup for the files can be excessive.
The solution is to use a DeferredImageDescriptor that just represents the file name, and then use the getImageData call to do the Platform.find() lookup at that point. It would mean that an error in specifying the image wouldn't be visible until the image file data was requested (right now, an error would be displayed if it tried to load any of them I think - or at least, a red image square) but this could be mitigated by tests.
Admittedly, on my test system the initializeImages() only takes ~25ms in total (so there's not likely to be a huge win) but it's on the critical path for startup. To put things in perspective, this change: // URL url = BundleUtility.find(PlatformUI.PLUGIN_ID, path); URL url = new URL("platform:/plugin/" + PlatformUI.PLUGIN_ID + "/" + path); in WorkbenchImages::declareImage drops the time from ~25ms to ~10ms. Part of that's due to the reduction in method calls, but mainly it's to do with the fact we're not hitting bundle.getEntry() on a whole bunch of those file names. I'm not suggesting this is necessarily the best fix (we don't support $os$ or similar with the above hack) but just documenting the kind of improvement that might be possible.
BTW when I was testing against the .jar the declareImages was taking ~50ms to run. I suspect that's because bundle.getEntry() has to do more work when it's a JAR, but when running in debug in the workspace it's just mapping the calls down to File calls which thus can be executed faster (it's relying on the OS to navigate the filesystem instead of Java which will navigate the JAR entry)
Deferring would also make sense if providing replacement images in a fragment…
I had a proof-of-concept for the deferred images using a wrapper: private final static void declareImage(String key, String path, boolean shared) { URL url = BundleUtility.find(PlatformUI.PLUGIN_ID, path); ImageDescriptor desc = ImageDescriptor.createFromURL(url); declareImage(key, desc, shared); } replaced with something along the lines of: private final static void declareImage(String key, String path, boolean shared) { ImageDescriptor desc = new DefferedImageDescriptor(String bundle, String path); declareImage(key, desc, shared); } static class DefferedImageDescriptor { DefferedImageDescriptor(String bundle, String path) { this.bundle = bundle; this.path = path; } ImageData getImageData() { return ImageDescriptor.createFromURL(BundleUtility.find(PlatformUI.PLUGIN_ID,path).getImageData(); } } That way the lookup is deferred until the getImageData is called.
BTW I note that JFace has some similar lookups in JFaceResources.initializeImages and JFaceResources.declareImage private static final void declareImage(Object bundle, String key, String path, Class<?> fallback, String fallbackPath) { ImageDescriptor descriptor = null; if (bundle != null) { URL url = FileLocator.find((Bundle) bundle, new Path(path), null); if (url != null) descriptor = ImageDescriptor.createFromURL(url); } // If we failed then load from the backup file if (descriptor == null) descriptor = ImageDescriptor.createFromFile(fallback, fallbackPath); imageRegistry.put(key, descriptor);
(Or to put it another way, a deferred JFace image lookup class might be worth putting in JFace, and then use that standard in both JFace and other dependent classes)
(In reply to Alex Blewitt from comment #8) > (Or to put it another way, a deferred JFace image lookup class might be > worth putting in JFace, and then use that standard in both JFace and other > dependent classes) +1. Can you please create a new bug for that and potentially provide an implementation for it?
Yeah, it's on the backlog ...
(In reply to Lars Vogel from comment #9) > (In reply to Alex Blewitt from comment #8) > > (Or to put it another way, a deferred JFace image lookup class might be > > worth putting in JFace, and then use that standard in both JFace and other > > dependent classes) > > +1. Can you please create a new bug for that and potentially provide an > implementation for it? Adding Stefan, as he might also be interested in this.
Created bug 477391 for the JFace enhancement.
No plans for me to work on this at the moment, re-assigning to inbox.
*** This bug has been marked as a duplicate of bug 477391 ***
This depends on bug 477391, it's not a dupe. I'd like to give Sergey the appropriate credit for fixing it.
Reopened since bug 477391 was reverted.
New Gerrit change created: https://git.eclipse.org/r/163890
I saw the issue with the Pin Editor, so made a change to the MenuHelper to fix the issue, and verified that that specific menu item is fixed. I don't know if there might be any other knock-on effects that I'm unsure about. https://git.eclipse.org/r/#/c/163890/1/bundles/org.eclipse.ui.workbench/Eclipse+UI/org/eclipse/ui/internal/menus/MenuHelper.java
I guess this may make a bigger impact on Windows, AFAICS file system access is super slow on Windows. Alex, please set milestone and assign bug to you.
Gerrit change https://git.eclipse.org/r/163890 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c60b87a6bd828ca13543e38548919a7ed2e46446
Thanks, Alex.
