Summary: | Accelerator Menu performance problems (GTK & Motif) | ||||||
---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Chris McLaren <csmclaren> | ||||
Component: | UI | Assignee: | Chris McLaren <csmclaren> | ||||
Status: | RESOLVED FIXED | QA Contact: | |||||
Severity: | critical | ||||||
Priority: | P1 | CC: | airvine, eduardo_pereira, James_Moody, Mike_Wilson, n.a.edgar, steve_northover | ||||
Version: | 2.1 | ||||||
Target Milestone: | --- | ||||||
Hardware: | PC | ||||||
OS: | Windows 2000 | ||||||
Whiteboard: | |||||||
Attachments: |
|
Description
Chris McLaren
2002-10-28 15:24:44 EST
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. |