Bug 219432 - [JFace] ImageDescriptor should use SWT's faster operating system image loading
Summary: [JFace] ImageDescriptor should use SWT's faster operating system image loading
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 M6   Edit
Assignee: Tod Creasey CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on: 219645
Blocks:
  Show dependency tree
 
Reported: 2008-02-19 09:12 EST by Chris Grindstaff CLA
Modified: 2008-04-23 11:32 EDT (History)
6 users (show)

See Also:


Attachments
Snippet of comparing ImageDescriptor vs new Image (1.02 KB, text/plain)
2008-02-20 12:14 EST, Chris Grindstaff CLA
no flags Details
Work in Progress (12.92 KB, patch)
2008-02-20 13:48 EST, Tod Creasey CLA
no flags Details | Diff
New patch (8.46 KB, patch)
2008-02-25 16:17 EST, Tod Creasey CLA
no flags Details | Diff
R 3.3.2 version (10.46 KB, application/octet-stream)
2008-04-22 14:47 EDT, Tod Creasey CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Grindstaff CLA 2008-02-19 09:12:54 EST
Build ID: I20080207-1530

Steps To Reproduce:
FileImageDescriptor currently loads its image data from the file system using the an inputstream. This pulls the image loading code into Java instead of using SWTs higher performance APIs. 

The FileImageDescriptor code should be changed to use the faster operating system image loading from a file name if possible. new Image(Device, String) for example.

In a large RCP product making this change in application code resulted in a 800ms improvement to blitting a large PNG with transparency.


More information:
Comment 1 Tod Creasey CLA 2008-02-20 10:12:32 EST
Chris can you give an idea of the code you were using to determine this improvement? It sounds like a good test case to me.
Comment 2 Chris Grindstaff CLA 2008-02-20 12:14:24 EST
Created attachment 90209 [details]
Snippet of comparing ImageDescriptor vs new Image
Comment 3 Chris Grindstaff CLA 2008-02-20 12:15:17 EST
Tod - I assume a stand-alone RCP example would be prefereable, is that right?
In the interest of responding sooner rather than later, I attached the snippet of code I used.
Comment 4 Tod Creasey CLA 2008-02-20 12:58:13 EST
I want to add a test suite for this so I don't even need an RCP app - a JFace performance test is a good start.
Comment 5 Tod Creasey CLA 2008-02-20 13:48:01 EST
Created attachment 90231 [details]
Work in Progress

Here my current work on one that can support the current API without any changes but it is relying on the FileLocator which currently has Bug 219645 with it.

Chris I assume you are using this in an RCP app with OSGi loaded? This is the case where I can most likely use the optimizations from SWT as I will need to file locator to translate URLs.
Comment 6 Tod Creasey CLA 2008-02-21 12:20:02 EST
Fixed to use the SWT ImageData loading in build >20080221.

I have written a performance test for this in org.eclipse.ui.tests.performance (FileImageDescriptorTest) but I will need some more complex images to get big enough numbers to see what kind of saving we get from this.

Chris can you give me an idea of how to whip up images that would we a lot faster with the new SWT processing for testing?
Comment 7 Tod Creasey CLA 2008-02-25 12:20:40 EST
Reopening - the current solution is not any faster.

Currently the solution to load image data with the filename rather than the Stream - Steve would this method take advantage of native loading?
Comment 8 Tod Creasey CLA 2008-02-25 16:17:51 EST
Created attachment 90692 [details]
New patch

This patch reverts getImageData to the old behaviour but optimized the createImage call. It is about 40% faster in Chris's test case but my suite needs some work to verify.
Comment 9 Steve Northover CLA 2008-02-25 16:36:35 EST
> Currently the solution to load image data with the filename rather
> than the Stream 

Yes, it should be faster.  To be clear, in the end, you will be calling new Image(Device, String) rather than new Image (Device, InputStream), right?
Comment 10 Tod Creasey CLA 2008-02-26 08:11:37 EST
Correct.
Comment 11 Tod Creasey CLA 2008-02-26 09:08:35 EST
New patch released for build >20080227. Chris if you could verify with your local test case I would appreciate it.

Note that the released version does not change how ImageData is loaded but overides the ImageData based loading from ImageDescriptor with a direct call to the file based API.
Comment 12 Randy Hudson CLA 2008-02-26 12:18:55 EST
Does this mean that most plug-ins will not benefit from faster loading times, since images are located inside of JARs?  What about descriptors created from URLs that are local files?
Comment 13 Tod Creasey CLA 2008-02-26 12:56:54 EST
Not it means calls to getImageData will not be affected. We are no longer using the ImageData to load images from a FileImageDescriptor so you should see improvements both within and external to OSGi.

See FileImageDescriptor #getFilePath if you want to see the implementation of the file lookup
Comment 14 Randy Hudson CLA 2008-02-26 13:16:21 EST
We're loading images using:

URL url = FileLocator.find(bundle, iconPath, null);
imageDescriptor = ImageDescriptor.createFromURL(url);

It seems like this wouldn't result in a FileImageDescriptor.
Comment 15 Tod Creasey CLA 2008-02-26 14:06:13 EST
Correct - that is a URLImageDescriptor
Comment 16 Tod Creasey CLA 2008-03-25 16:40:15 EDT
verified in I20080325-0100
Comment 17 B. Chen CLA 2008-04-21 14:47:06 EDT
Hi Tod, could you please provide patch for Eclipse 3.2.2 Jface? Our product is based on Eclipse 3.2.2, we would like to back port this fix to 3.2.2
Comment 18 Tod Creasey CLA 2008-04-22 14:47:54 EDT
Created attachment 97070 [details]
R 3.3.2 version

R3.3.2 version using the R 3.3.2 error reporting
Comment 19 B. Chen CLA 2008-04-23 11:32:12 EDT
Hi Tod, Is the patch you provide for Release 3.3.2 or 3.2.2? Our product is based on 3.2.2, we would appreciate if you could provide patch for 3.2.2. Thanks!