X server roundtrips positioning invisible popups can hurt performance

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

In some work I did a couple of years ago, I made popups be toplevel widgets and added code to position them here:
http://lxr.mozilla.org/mozilla/source/view/src/nsView.cpp#374
This requires an X server roundtrip in general. This can get severe when we have a lot of popups --- in particular, each comboboxes has one --- and we reposition them frequently.
Created attachment 232821 [details]
testcase

This testcase demonstrates the problem.
Created attachment 232822 [details] [diff] [review]
fix

This patch basically fixes the issue. It improves the performance of the testcase on my machine (debug build!) by about 5x, from ~4700 to ~900 ms.
Attachment #232822 - Flags: superreview?(bzbarsky)
Attachment #232822 - Flags: review?(bzbarsky)
Comment on attachment 232822 [details] [diff] [review]
fix

Makes sense.
Attachment #232822 - Flags: superreview?(bzbarsky)
Attachment #232822 - Flags: superreview+
Attachment #232822 - Flags: review?(bzbarsky)
Attachment #232822 - Flags: review+
Created attachment 232846 [details] [diff] [review]
fix

Sorry Boris, I think we should go with this more conservative fix instead, which restricts the optimization to popups. I'm afraid of messing up WidgetToScreen calls applied to child widgets.
Attachment #232822 - Attachment is obsolete: true
Attachment #232846 - Flags: superreview?(bzbarsky)
Attachment #232846 - Flags: review?(bzbarsky)
Comment on attachment 232846 [details] [diff] [review]
fix

Ah, good catch.
Attachment #232846 - Flags: superreview?(bzbarsky)
Attachment #232846 - Flags: superreview+
Attachment #232846 - Flags: review?(bzbarsky)
Attachment #232846 - Flags: review+
Created attachment 232859 [details] [diff] [review]
yet another try

Dear oh dear. The last one was borked because it didn't set "offset" for non-popup windows. hopefully this is it...
Attachment #232846 - Attachment is obsolete: true
Attachment #232859 - Flags: superreview?(bzbarsky)
Attachment #232859 - Flags: review?(bzbarsky)
Comment on attachment 232859 [details] [diff] [review]
yet another try

r+sr=bzbarsky, but are you sure you still trust my reviews?  :(
Attachment #232859 - Flags: superreview?(bzbarsky)
Attachment #232859 - Flags: superreview+
Attachment #232859 - Flags: review?(bzbarsky)
Attachment #232859 - Flags: review+
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.