Community
Participate
Working Groups
Created attachment 192422 [details] First draft of performance improvements Lately I had to do some work on the RAP UI tests. Unfortunately I discovered that the suite was performing maddening slowly - it took about 40 minutes to run on my machine. So I started a measurement session and found out that there were over 45.000.000 calls to one of the TextSizeEstimation#xxxExtend methods for around 1.500 different strings. This lead to tremendous CPU usage. In fact the first time I had run the suite I killed the execution thinking there was an endless loop as the suite didn't move on. Introducing the buffer that you can see in the attachment improved the situation. The suite now took about 8 minutes to finish. But this solution feels a little like treating the symptom only and not the origin of the problem. So I did some further investigations and found out that the predominant amount of estimation requests were caused by the Tree#needsHScrollBar() method. This is understandable considering that recalculation is necessary each time a new tree item is created, an existing tree item gets disposed of or a size changing method like TreeItem#setText() gets called. The jface test suite does a lot of tree testing with trees that consist of three levels with 10 child items on each item of such a level. This explains why the suite operates so slowly. Thinking of the problem I came to the conclusion that although adding a new TreeItem for example forces a size recalculation for scrollbar adjustment there's no need to recalculate all the existing items. The size of existing items haven't changed since the last calculation. So I came up with the idea of buffering the size on each item. Tree size calculations would use buffered item sizes if existing and only would calculate them otherwise. The buffer itself gets cleared of course if a potentially size changing method on the item got called. The implementation provided does only a buffering of the preferred width value. This may seem somewhat unbalanced but actually I couldn't prove performance problems with the preferred height calculations. I didn't want to be more invasive as necessary so I omited buffering of the preferred height. But this may needs further investigations. Having both changes in place the UI test suite now takes less than 4 minutes to run on my machine. Although this is quite an improvement we still have to think about the relevance in practice. The problems are caused by the TextSizeEstimation. But in a real world system the TextSizeDetermination would have taken over after the first time exact calculation values for the given strings were available. So the effect probably is not that huge in real systems. But on the other hand the UI test suite speedup is also an important point... One last thought on the TextSizeEstimationBuffer implementation and usage. The proposed implementation does synchronization in the TextSizeDetermination#estimate method. While this is the proper implementation it makes the code somewhat less readable. I would rather prefer having the synchronization in the buffer implementation. This is possible since the worst thing that could happen is that there may be a few redundant calculations putting the same value twice into the map. So what do you think? And is it possible that we have a similar problem with Table and List controls?
Is this related to Bug 340839?
(In reply to comment #1) > Is this related to Bug 340839? I think this is possible. Maybe Kaloyan Radev could use the attached patch and see whether this improves his situation.
I've tried the patch and the performance improvement suggested here takes loading of large data (1200 elements) trees, not having columns, from about 150 seconds down to 5 (!) seconds, which actually means it brings great acceleration in my opinion. As for the relation with Bug 340839 the things look quite different - the tree that I had actually is having multiple columns so caching the cell widths does not help (the column withs, not the cell ones are taken into account for the horizontal scroll). Anyway while trying the patch I realized few things - first the behavior in 1.4M6 (as I was on 1.3.2 before that) is so much better, so that the slow loading situation is no longer valid - even if I hit something unreal like 100K elements in non-lazy provider table it takes less than 30 seconds. And the other thing is that the 70-80% CPU in Tree#updateScrollBars() mentioned there were brought mostly from profiler overhead, so probably I'll invalidate the issue after some more measurements.
Created attachment 193353 [details] Patch introduces preferredWidthBuffer on treeItem After doing some further investigations it looks like the text size estimation result buffering is not necessary once the tree items buffer the preferred width values. The hit rate in the estimation result buffer went down to 30.000 and there was no measurable performance break-in running the UI Test Suite. So I reworked the latest changes a bit and attached a new patch that only changes Tree related classes. I marked those changes related with this issue with a //TODO [DISCUSS_PERFORMANCE] comment in the patch. If you find some time please review those changes.
Created attachment 193522 [details] Cleaned-up version of the previous patch I committed code cleanup and refactorings of the affected classes to CVS HEAD and created a new version of the patch that only captures the actual changes suggested in this bug.
I found that with the latest patch (#3) the Tree items in the workbench demo (startup=default) are cut off. Maybe the change does not work correctly with Trees that don't have columns?
(In reply to comment #6) Actually, I've seen this behavior in forms (FormText), when some sizes are cached in the first layout (text estimation), but not updated in the resize event, triggered by text size determination with the actual text dimensions. Maybe we can consider here this bug as well - bug 329881.
Created attachment 193568 [details] Patch that solves problem described in comment 6 Ouch! I can not believe that I missed this problem. But this attachment (hopefully) solves it. I adjusted the test accordingly.
I think Ivan made an interesting point in comment 7. Since the buffers are not cleared on resize, the estimated (think "wrong") values will never be replaced by measured (think "right") ones during the lifetime of the Tree in the first session unless certain item operations are done. This is just an assumption, maybe I'm wrong. If this assumption is true, would it make sense to clear the buffers also in the Tree.ResizeListener in order to react to the TSD resize? Assumed that looking up values from the measurement store is cheaper than estimating, this should not involve a performance penalty. What do you think?
Created attachment 193791 [details] Solution for appliance of measurement values problem The attachment solves the problem described by Ralf in comment 8, but the solution is different as the suggestion in the same comment. For me it feels somewhat wrong to clear the preferredWidthBuffer of tree items each time a resize event on the tree occurs. In most cases there would be no need to do this. Instead I've overridden the Composite#change(Control[]) method in Tree. I think the semantical context of this method fits better into our problem domain. It is also called by the mechanism that applies the 'right' text size values to the available widgets and should do the trick. What do you think?
Thanks for the updated patch. I agree that using Control#change() is a good fit. Applied patch #5 (attachment 193791 [details]) to CVS HEAD.
