Community
Participate
Working Groups
GTK and Motif have significant performance problems using the hidden menu in its current form. I looked at a profile and found that at least 700ms is spent in AcceleratorMenu.setAccelerators on these platforms (every time we recalculate the menu - which can be as often as switching editors). I am hoping that this has to do with the fact that the hidden menu is being disposed of and recreated with new menu items each time setAccelerators is called. Can someone look at this, and perhaps reuse MenuItem instances if that proves to be the problem?
This is a P1. Chris to assist SN constructing a benchmark.
Created attachment 2292 [details] The benchmark code
Results: Windows 98, PII-433-256 - 110ms Motif RedHat PII-450-256 - 438ms GTK RedHat 8.0 PIII-500-512 - 180ms These times don't look that bad, given the machines I am running on. Do you want me to investigate further? Is it possible that the UI code is calling this API multiple times instead of just once when switching between editors?
If that benchmark really is indicative of the problem, then Motif should be taking even longer to switch pages than GTK. Thus either the benchmark isn't a good indicator, or the problem is somewhere else. We need to find out which of these is true. This is still P1 in SWT. If SN isn't going to do the work to figure out what's going on, he should assign it to UI so they can find someone to do it.
I can't really look at this now. Here's what I suspect. I believe that the problem is that UI does a whole bunch of work on Acivate/Deactivate. This happens on GTK when a menu loses focus. It didn't use to happen (we had code that avoided this in Menu) but the code was a mess and introduced bugs. Taking it out fixed bugs. We can put the code back if necessary. UI needs to confirm that they are calling this API only once when switching editors.
We investigated the performance problem and found that AcceleratorMenu.setAccelerators was being called repeatedly with the same list of accelerators. The proposed fix is to check for the case where the lists are the same and do nothing. Below is a modified version of setAccelerator which does this. There are benchmark checks included as well. With this fix, calling setAccelerator when the items are the same takes less than a millisecond. When switching between Java editors, the list is always the same. Also, there was a null problem in the code which I fixed: //benchmarking int count = 0; // public void setAccelerators(final int[] accelerators) { //benchmarking long start = System.currentTimeMillis(); // if (accelerators == null) { this.accelerators = null; } else { if (this.accelerators != null && this.accelerators.length == accelerators.length){ int index = 0; if (accelerators.length > 0) { while (index < accelerators.length) { if (this.accelerators[index] != accelerators[index]){ break; } index++; } } if (index == accelerators.length){ //benchmarking long end = System.currentTimeMillis(); count++; System.out.println(count + "setAccelerators - no change"); System.out.println(count +"setAccelerators time = "+(end - start)); // return; } } this.accelerators = new int[accelerators.length]; System.arraycopy(accelerators, 0, this.accelerators, 0, accelerators.length); } menu.dispose(); menu = new Menu(parent.getParent(), SWT.DROP_DOWN); if (item != null) item.setMenu (menu); if (accelerators != null) { for (int i = 0; i < accelerators.length; i++) { final int key = accelerators[i]; MenuItem keyMenuItem = new MenuItem(menu, SWT.PUSH); keyMenuItem.setAccelerator(key); keyMenuItem.addListener(SWT.Selection, menuItemListener); } } //benchmarking long end = System.currentTimeMillis(); count++; System.out.println(count + "setAccelerators length = "+((this.accelerators == null) ? 0 : this.accelerators.length)); System.out.println(count +"setAccelerators time = "+(end - start)); // }
The UI team will need to investigate the six calls. Shall we track this under the same PR or a different one? Added Nick to the CC
Reassigning to UI to investigate the 6 calls, and apply the changes above if they haven't been already.
*** Bug 25777 has been marked as a duplicate of this bug. ***
added this line to the beginning of setAccelerators(..) method: if (java.util.Arrays.equals(this.accelerators, accelerators)) return; committed to head nov 13 10:35am regarding 6 calls: will investigate in workbench, but performance issues should largely disappear. another performance issue was already addressed (preoptimizing keybindings for platform and locale), and further specific optimizations in the keybinding engine will be committed shortly.
