Community
Participate
Working Groups
Our product intensively uses SWT to represent results to user. It may create hundred widgets. Sometimes it uses Control.update() method with "all" set to false. This method takes about 20% of entire time taken by SWT. I profiled the use case and noticed that Display.checkIfEvent() is called about 8 200 times however Control.update() was called only 16 times. I also noticed that Display.checkIfEvent() modifies event in the queue (Do not know whether this is a good practice). This is due to each event in the X queue is checked each time Control.update() is called. As far as I understand the goal of this function is to collect all pending Expose events for given widget and optionally for all its children and then process all updates at once. I tried to modify Control.update() method to use XCheckWindowEvent() and got good results. This method is slightly faster if "all" argument is true and takes almost no time if "all" is false. VisibilityNotify events are removed from the queue so they are not processed again by GTK. Could you please review the patch and give your opinion. Is it possible to replace current SWT implementation with proposed one?
Created attachment 16264 [details] Control patch
Created attachment 16265 [details] Native library for patch This library implements methods from OS1 class.
I think using XCheckWindowEvent instead of XCheckIfEvent is a good idea. It doesn't require a callback and it only does the work on the window in question. Silenio, what do you think ?
Felipe, make the changes that use XCheckWindowEvent() when "all" is true. I am not sure the other case is worth. What are the improvements? Also, it misses windows that are not known by GDK (gdk_window_peek_children()).
Why does your produce call update()? This method is slow on Windows too because it defeats the merging of outstanding paint events that is done by the operating system. Can you tell us where it is being called from?
To comment #4 Main advantage of proposed implementation is when “all” is false. User requests update on particular control, so there is no need to scan all events. When “all” is true we also check for events for children. The main advantage is that it removes Expose and Visibility events from the queue, so GDK does not deal with them again. Curren implementation simply corrupts them by setting type to -1. “Also, it misses windows that are not known by GDK (gdk_window_peek_children())” Current implementation also misses non GDK windows, it does lookup of parent and if parent lookup does not result in requested window it does nothing. To comment #5 We call it from our custom widgets with “all” set to false to ensure all pending paint requests on Canvas are completed. This is not a problem of SWT. Another interesting point is that Control.update(true) is called from Shell.open (). And this method takes 95% of Shell.open() procedure because of gtk_window_process_updates(). I just commented it and method time became 97 msec vs 1800 msec in original (Main eclipse Shell open time). Visually the results were the same. The question is do we really need Control.update(true) method in Shell.open()? GDK will do this for us when main loop is idle.
Created attachment 16319 [details] Control&Display patch I updated the patch to do extra checks and moved the method into Disply so implementation is more consistent with current one.
Artyom, I've load your code into my workspace in a way I have both version of the code running (your implementation and the current one). I also loaded the ControlExample from org.eclipse.swt.examples and added the following code to CanvasTab#createOtherGroup(): Button testButton = new Button (otherGroup, SWT.PUSH); testButton.setText("TEST"); testButton.addSelectionListener (new SelectionAdapter () { public void widgetSelected (SelectionEvent event) { long time; int count = 2000; time = System.currentTimeMillis(); for (int i = 0; i < count; i++) { canvas.redraw(); canvas.update(); } time = System.currentTimeMillis() - time; System.out.println("Time " + time + " (ms)"); time = System.currentTimeMillis(); for (int i = 0; i < count; i++) { canvas.redraw(); canvas.updateArtyom(); } time = System.currentTimeMillis() - time; System.out.println("Time (Artyom) " + time + " (ms)"); } }); Your code is only 5%, maybe 10%, faster than the old code. Is this the scenario where I get the best performance from your code (all == false) ? Could you write a testcase (using System.currentTimeMillis()) that I can use to compare your new implementation against the old one and see how much faster your code is ? Thanks.
I just look at the patch a little closer and I remember why we are using XCheckIfEvent instead of XCheckWindowEvent. XCheckWindowEvent removes the events from the X queue, which means GDK will not see them. This is "OK" for the Expose events, because we are basically doing the same work GDK does (add the damage to the GDK update region and then in idle handler Expose evnts are generated by GDK and dispatched to the widgets). But removing the VisibilityNotify is not OK, because widgets that add a signal handler to "visibility-notify-event" will not see them anymore.
To comment #8 I profiled real application based on Eclipse (Our product which creates a lot of controls) using JProbe profiler. I doubt that custom benchmarks will show this problem because X queue is always empty or very small. As far as I understand the goal of Control.update() method is to perform all operations scheduled by gdk_window_invalidate_region() and also scan X queue so all Exposes (client initiated and server initiated) are processed at the same time. In your benchmark canvas.redraw() only schedules repaint using gdk_window_invalidate_region() so no actual X events are sent to the server. Actual events will be sent to the server when canvas.update() will call gdk_window_process_updates(). But at this moment there is no server initiated expose events in X queue. To comment #9 The code removes Visibility events only for windows that correspond to SWT Controls, who else may hook into “visibility-notify-event” for these widgets. So we can do the same actions like we do in gtk_visibility_notify_event(), i.e. call redrawWidget() method if necessary.
Felipe's benchmark is definitely incorrect, the case to worry about is when there are outstanding paints from the X server. We should get a better benchmark. :) That said, if we can get rid of places where we call update() unnecessarily, I think we will be much better off. The method should really be avoided except for during scrolling or interactive resizes (where the X event queue will be small anyway). I opened bug 80276 and bug 80277 about two places I think are unnecessary, one of them being the Shell.open() case you mention. I agree that it should probably be removed unless absolutely necessary.
I agree with Billy that it is better to remove update() method from places where it is not completely necessary in SWT. But our product needs this method in any way, so it would be great if it was rewritten also. Is explanation about why we can remove visibility events from the X queue OK? Do you need a patch which incorporates these changes?
If any of the widgets default signal handlers for "visibility-notify-event" (not the SWT ones) do anything, we would be missing that.
I see the concern. I checked current GTK 2.4 source and found no default handlers for "visibility_notify_event". Probably we can assume that there are no default handlers :-) Another thing we can do is put visibility events back using XPutBackEvent.
Created attachment 16856 [details] Display&Control&Shell patch Here is new version of the patch which places visibility-notify-events back to X queue. I also added Shell.update(boolean) method which behaves differently than Control.update(bollean). Since Shell (especially main eclipse Shell) has a lot of controls it is better to scan throw entire X queue rather than do recursive checks. Could you please review?
Created attachment 16857 [details] Native library for patch Updating native library
Even better would be to just remove the code which checks VisibilityNotify messages.
Does this patch provide a real win? Is the problem with visibility-notify-events purely hypothetical? If there are no known instances of places where it causes a problem, can we not just do the fast thing, and (if absolutely necessary) have an option to provide the slow behavior?
This method is quite expensive. This is very significant when it is necessary to call update on Canvas. I.e. only Canvas widget should be updated but current code scans ALL events in the queue. Our product uses Canvas for custom widgets (i.e. we draw them using GC) and we need to call update before scrolling. As the result scrolling is slow since each update() call scans entire X queue. Attached patch fixes this.
Created attachment 20220 [details] Patch for just the case where all = false Here is a more conservative version of the patch which catches the case where all=false. It gives about an 8% performance improvement to a simple StyledText scrolling benchmark. To try and stress the case where there are X events in the queue, I ran my scrolling benchmark while an always-on-top window was obscuring the scrolling area, and while I moved my mouse around over the window to generate motion events. In this patch, I simply ignore visibility events. I think this is safe in this case since in the worst case, we cause a paint even though the window will become hidden. I do not think this is worth adding extra complexity to handle. Silenio, am I missing something here?
Unfortunately, I forgot a call to gdk_flush() in my last patch which makes the performance gains less significant in my test case.
In profiling with sysprof and oprofile (highly recommended, BTW), I found that when the event queue is long, we can save time by only accessing the event type rather than the whole event structure. Since this is now measurably faster, and given that my attempts at doing a simpler patch (comment #20) have failed, I think it's time to close this bug unless there is fresher profiler data showing how to improve it further. Please test with builds > 20051112 and see how much the profiles improve.
