Community
Participate
Working Groups
Created attachment 93466 [details] Attempts to suggest optimizations for Win32 Tree#setImage Build ID: I20080207-1530 The attached patch attempts to suggest optimizations for the Win32 TreeItem#setImage that: - speed up Tree#imageIndex(Image,int) and - decreases the number of Widget#checkWidget(). Note that the optimization of Tree#imageIndex doesn't pretend to be complete for all uses of Tree and it just reflects my limited experience with the related code and the case of Tree with TreeColumns that are not reordered. I think the removed calls to Widget#checkWidget() are reasonable even if the said method doesn't take noticeable time because the affected methods don't directly need the underlying handles and just delegate to a method that makes the checks it needs. The following draft of a table shows a comparison of the performance of the combined invocations of the three TreeItem#setImage methods (i.e. an invocation of TreeItem#setImage(Image) is counted as well as the invocation of TreeItem#setImage(int,Image) the former performs): time (ms) unpatched 1923 patched 786
Can you attach the benchmark code? Thanks.
Note that the first line of every public method must be checkWidget() to ensure that we get this right across platforms (from experience). Also, for methods that take an argument that is an object, checkWidget() is performed before a null check. If this really turns out to be a problem, we would write and _setImage() that did everything but the checkWidget() and then call that or more likely, we would inline the code to squeeze out every last millisecond.
(In response to comment #2) Thank you very much for your attention! I hope I didn't mislead you into believing that the Widget#checkWidget calls inside TreeItem#setImage are the culprit. As I noted, I thought the modifications were reasonable regardless of the time taken by Widget#checkWidget. For example, TreeItem's "public void setImage(Image image) { checkWidget(); setImage(0, image); }" doesn't perform null checks and just delegates to setImage(int, Image) which, as a public method itself, performs the necessary checks in accord with the convention you described. In this case, there're two calls to Widget#checkWidget one after the other. Obviously, my suggestion cannot be applied to TreeItem#setImage(Image[]) in the light of the outlined convention because it breaks the rule "checkWidget() is performed before a null check". I guess TreeItem#setImage(Image[]) can then delegate to _setImage(int, Image) (as you mentioned) instead. In a summary, the real performance penalty is Tree#imageIndex(Image,int) (in my opinion, of course) and the modifications regarding the Widget#checkWidget calls are finishing touches, regardless of their penalty status, on the quest of getting better execution times from TreeItem#setImage.
Created attachment 94521 [details] Allows testing TreeItem#setImage through loading Files into a Tree The attached code allows loading a folder hierarchy into a Tree and uses the Image associated through Program to call TreeItem#setImage for each File represented as a TreeItem. The code relies on TreeItem defining: public static long setImage_invocations; public static long setImage_time; Each of the three TreeItem#setImage methods is modified to: - begin with code that counts the invocations and sets up the start time of the invocation: setImage_invocations++; long start = System.currentTimeMillis(); - acknowledge the time spent in the invocation upon return (TreeItem#setImage(int,Image) has multiple points of return so each of them is modified the contain the code mentioned bellow) through: setImage_time += (System.currentTimeMillis() - start);
We'll focus on Tree#imageIndex(Image,int). Stand by ...
Could I trouble you for a main()? Sometimes the size of the main shell makes a difference in benchmarks and I want to make sure we are running exactly the same thing.
Also, for the same reason (exactly the same code), can you please paste in your TreeItem.setImage() method(s)?
Created attachment 94629 [details] Hopes to provide a more complete test application for TreeItem#setImage Thank you for your patience and dedication! The attached ZIP expands to a folder named 223944 (after the number of this bug report) which contains the requested (at the very least, my understanding of and attempt at the requested) files. (In response to comment #6) Well... As I work on a RCP application, my benchmark code is in the form of an application plug-in (so that I can easily use and modify the org.eclipse.swt.win32.win32.x86 plug-in). Because you said you wanted to run exactly the same thing, I provide the source of the mentioned application plug-in in the attached ZIP as 223944\com.deltopia.ui.tests. The size of the main Shell is dynamically calculated depending on the Display size. Which may turn out to be of help if one's in doubt that the benchmark results depend on the Shell size because I've run the test multiple times on Windows Vista with a Display size of 1680x1050 and on Windows XP with a Display size of 1024x768 and the ratio of the unpatched to the patched code in terms of execution times seems to me to not depend on the Display size. Additionally, the suggested changes were initially implemented for and timed multiple times on the RCP application I work on and its main Shell has the default size chosen by the Eclipse workbench so the Tree size is comparable to it. (In response to comment #7) The changes were initially applied to the SWT source bundled with Eclipse 3.4m5 and the average times reported in the initial bug description were observed on it. Upon your request for the benchmark code, I re-checked and re-confirmed the execution-time ratio on the SWT source shipped with Eclipse 3.4m6. Thus I'm providing TreeItem#setImage with time probes in the form of 223944\org.eclipse.swt.win32.win32.x86\src\org\eclipse\swt\widgets\TreeItem.java in the attached ZIP, the original source of which is taken from the SWT source available in Eclipse 3.4m6.
I have the code now and am running it. It's a nice little application but as a benchmark, I'm not so sure. Please don't be offended as you have done a great job building it and tracking down problems. Let's continue. Here are the results of running the new and old code on C:\TEMP: NEW CODE: new TreeItem= { invocations= 18020, time= 2778 } TreeItem#setImage invocations in FolderTree= 27664 new TreeItem= { invocations= 18020, time= 2562 } TreeItem#setImage invocations in FolderTree= 27664 new TreeItem= { invocations= 18020, time= 2794 } TreeItem#setImage invocations in FolderTree= 27664 OLD CODE: new TreeItem= { invocations= 18020, time= 2970 } TreeItem#setImage invocations in FolderTree= 27664 new TreeItem= { invocations= 18020, time= 2555 } TreeItem#setImage invocations in FolderTree= 27664 new TreeItem= { invocations= 18020, time= 2761 } TreeItem#setImage invocations in FolderTree= 27664
It seems that your code avoids the tests that determine whether the image list needs to be set. The image list itself is only set once (this is expensive and causes a redraw, even when set to the same thing). I need to understand the code a bit more to see whether your change is harmful (probably not). If the change is good, I'll apply the concept to every place where we compute the image list index.
Here is a benchmark: package steve; import org.eclipse.swt.*; import org.eclipse.swt.graphics.*; import org.eclipse.swt.layout.*; import org.eclipse.swt.widgets.*; public class PR_223944a { public static void main(String[] args) { final Display display = new Display(); int s1 = 16; final Image image = new Image (display, s1, s1); try { GC gc1 = new GC (image); gc1.drawRectangle (0, 0, s1 - 1, s1 - 1); gc1.setBackground (display.getSystemColor (SWT.COLOR_GREEN)); gc1.fillRectangle (1, 1, s1 - 2, s1 - 2); gc1.dispose (); Shell shell = new Shell(display); shell.setLayout(new FillLayout()); Rectangle bounds = display.getBounds(); int height = bounds.height / 2; int width = bounds.width / 2; int x = bounds.x + (bounds.width - width) / 2; int y = bounds.y + (bounds.height - height) / 3; shell.setBounds(new Rectangle(x, y, width, height)); final Tree tree = new Tree(shell, SWT.NULL); int columnCount = 0, itemCount = 4, imageCount = 1000000; tree.setHeaderVisible(columnCount > 0); tree.setLinesVisible(true); for (int i = 0; i < columnCount; i++) { TreeColumn column = new TreeColumn(tree, SWT.NONE); column.setText("Column " + i); } for (int i = 0; i < itemCount; i++) { TreeItem item = new TreeItem(tree, SWT.NONE); long t0 = System.currentTimeMillis(); item.setText("Item " + i); for (int j = 0; j < imageCount; j++) { item.setImage (image); } long t1 = System.currentTimeMillis(); System.out.println("Time: " + (t1 - t0)); } for (int i = 0; i < tree.getColumnCount(); i++) { tree.getColumn(i).pack(); } shell.setText(tree.getClass().getName()); shell.open(); while (shell.isDisposed() == false) { if (display.readAndDispatch() == false) { display.sleep(); } } } finally { image.dispose(); display.dispose(); } } }
Here are my imageIndex() methods: int imageIndexNEW (Image image, int index) { if (image == null) return OS.I_IMAGENONE; boolean updateImageList; if (imageList == null) { Rectangle bounds = image.getBounds (); imageList = display.getImageList (style & SWT.RIGHT_TO_LEFT, bounds.width, bounds.height); updateImageList = true; } else { updateImageList = false; } int imageIndex = imageList.indexOf (image); if (imageIndex == -1) imageIndex = imageList.add (image); if (updateImageList && (hwndHeader == 0 || OS.SendMessage (hwndHeader, OS.HDM_ORDERTOINDEX, 0, 0) == index)) { int /*long*/ hImageList = imageList.getHandle (); int /*long*/ hOldImageList = OS.SendMessage (handle, OS.TVM_GETIMAGELIST, OS.TVSIL_NORMAL, 0); if (hOldImageList != hImageList) { OS.SendMessage (handle, OS.TVM_SETIMAGELIST, OS.TVSIL_NORMAL, hImageList); updateScrollBar (); } } return imageIndex; } int imageIndexOLD (Image image, int index) { if (image == null) return OS.I_IMAGENONE; if (imageList == null) { Rectangle bounds = image.getBounds (); imageList = display.getImageList (style & SWT.RIGHT_TO_LEFT, bounds.width, bounds.height); } int imageIndex = imageList.indexOf (image); if (imageIndex == -1) imageIndex = imageList.add (image); if (hwndHeader == 0 || OS.SendMessage (hwndHeader, OS.HDM_ORDERTOINDEX, 0, 0) == index) { int /*long*/ hImageList = imageList.getHandle (); int /*long*/ hOldImageList = OS.SendMessage (handle, OS.TVM_GETIMAGELIST, OS.TVSIL_NORMAL, 0); if (hOldImageList != hImageList) { OS.SendMessage (handle, OS.TVM_SETIMAGELIST, OS.TVSIL_NORMAL, hImageList); updateScrollBar (); } } return imageIndex; } int imageIndex (Image image, int index) { return imageIndexOLD (image, index); // return imageIndexNEW (image, index); }
Here are the results on my machine (for 1000000 iterations): NEW Time: 750 Time: 719 Time: 703 Time: 718 OLD Time: 4860 Time: 5047 Time: 5078 Time: 5047 NOTE: This makes no sense. I am doing something wrong.
Lord! I missed a zero. Ignore that last comment.
!@#$!@#%$!@#%$%$!@# Bugzilla $Q@#%
The new code causes a regression (bug 105494).
(In reply to comment #9) > I have the code now and am running it. It's a nice little application but as a > benchmark, I'm not so sure. Please don't be offended as you have done a great > job building it and tracking down problems. Let's continue. Thank you! I appreciate your attention and your remarks - positive or negative - are important to me. > Here are the results of running the new and old code on C:\TEMP: > > NEW CODE: > > new TreeItem= { invocations= 18020, time= 2778 } > TreeItem#setImage invocations in FolderTree= 27664 > new TreeItem= { invocations= 18020, time= 2562 } > TreeItem#setImage invocations in FolderTree= 27664 > new TreeItem= { invocations= 18020, time= 2794 } > TreeItem#setImage invocations in FolderTree= 27664 > > > OLD CODE: > > new TreeItem= { invocations= 18020, time= 2970 } > TreeItem#setImage invocations in FolderTree= 27664 > new TreeItem= { invocations= 18020, time= 2555 } > TreeItem#setImage invocations in FolderTree= 27664 > new TreeItem= { invocations= 18020, time= 2761 } > TreeItem#setImage invocations in FolderTree= 27664 The invocation counts given by the "new TreeItem= { invocations= X, time= Y }" lines will let us know how much TreeItems we're working with in case we want to pursue a doubt that TreeItem#setImage is unexpectedly affected by the number of TreeItems (I haven't noticed such unexpected dependency). The "TreeItem#setImage invocations in FolderTree= X" aren't all that interesting without the "TreeItem#setImage= { invocations= X, time= Y }" lines. The latter lines aren't present in the above output. As far as I understand it, the missing lines are of the greatest interest in the case of trying to measure the TreeItem#setImage optimization suggestion given in the patch because they contain the combined execution times of the three TreeItem#setImage methods. I guess they're not included because they require the "public static long setImage_invocations;" and "public static long setImage_time;" in TreeItem.java (as described in comment #4 and as provided in attachment #94629 [details]). The execution times in the initial bug descriptions reflect the time values of the "TreeItem#setImage= { invocations= X, time= Y }" lines. Additionally, given the invocation fields of both "TreeItem#setImage invocations in FolderTree= X1" and "TreeItem#setImage= { invocations= X2, time= Y }", one can determine how many (X2 - X1) additional calls to Widget#checkWidget (which is also described in the initial bug description and is contained in the patch and, though you said you would focus on the other issue, I had to provide it as well because you requested my benchmark so it wouldn't be fair on my side to change the benchmark while still reporting its previous times) one performs if calling TreeItem#setImage(image) instead of TreeItem#setImage(0,image). NOTE: By the time I finished writing my response to comment #9, I noticed you already found your own way of investigating the issue. But I'm providing my answer anyway because I respect you and I didn't want to risk offending you by not answering comment #9.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
