Closed Bug 450916 Opened 12 years ago Closed 12 years ago

Moving a canvas inside a stack via mouse events is very slow

Categories

(Core :: XUL, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: pavlov, Assigned: vlad)

References

Details

(Keywords: fixed1.9.1, mobile, perf)

Attachments

(3 files, 2 obsolete files)

This testcase runs fine on all my desktop machines but is horribly slow on the n810.
Attached file test javascript
Attached file test xul
Blocks: 436061
A few things are needed for best performance:

1) An X server built against a libpixman with both patches in bug 451621 applied.
2) Both of those patches applied to the mozilla libpixman (soon to be not needed, will get them checked in soon)
3) This patch applied to gecko
4) Running fennec with MOZ_NO_NATIVE_THEME=1 in the environment

Note that #1 isn't entirely required -- it's a good optimization, but even without it, the testcase is pretty snappy.  However, I have only tested against a recent X server and relatively recent unpatched pixman; Xomap is much older than both of those.

With all this stuff (with or without #1), the testcase here is completely responsive no matter how quickly I move the mouse.  Fennec feels a lot better, but it's still sluggish; I think there are some problems inside fennec itself that are causing that though, because profiles don't show us spending an unreasonable amount of time in painting (10% during my 60 second profiling run, which was constantly dragging around the content area).

This patch probably doesn't play nice with native theme itself; that should get fixed as much as possible in a followup patch, but I don't think it's possible to really fix it.  Our current native theme code depends on an X visual existing that matches the drawable depth that we want; that's just not true on these systems, where we only have a single 16bpp visual (and perhaps a 32bpp visual if COMPOSITE is available; I'm not sure if we can take advantage of that).  However, despite that, if we can get some kind of global MOZ_MOBILE_OPTIMIZE or something #define, we can conditionally do this stuff.
Assignee: jag → vladimir
Status: NEW → ASSIGNED
(We could turn native theme into a pref easily enough, too, to avoid the env var business)
Attached patch updated patch (obsolete) — Splinter Review
Adds a missing null check that otherwise causes glib warnings (harmless, in this case).
Attachment #337252 - Attachment is obsolete: true
this bug is a lot better if you tell gtk to redraw, but we should still profile and see if this patch helps.
Flags: blocking1.9.1+
Ok, here's an updated patch -- everything sits behind one-time-checked global prefs, one pref each for:

- forcing 24bpp mode for doublebuffer surfaces
- using a single pixmap instead of recreating it each expose
- disabling native theme
Attachment #337396 - Attachment is obsolete: true
Attachment #338904 - Flags: superreview?(pavlov)
Attachment #338904 - Flags: review?(jmuizelaar)
Attachment #338904 - Flags: review?(jmuizelaar) → review+
Updated the patch slightly locally -- it now passes the GdkDrawable in as the first arg to gdk_pixmap_new, like the old code did, to make sure that if all the prefs are the default the code is identical to the previous.
Attachment #338904 - Flags: superreview?(pavlov) → superreview+
Comment on attachment 338904 [details] [diff] [review]
Updated; use prefs for everything

The code here seems fine, but it is difficult to understand the impact of each of these prefs, and how they might interact with one another.  Which ones are optimal for the situation we're in?
In theory enabling all of them should help. However more testing of each of them is probably needed.
 nsWindow::GetSurfaceForGdkDrawable(GdkDrawable* aDrawable,
-                                   const nsSize& aSize)
+                                   const nsSize& aSize,
+                                   PRInt32 aDepth)

+        switch (aDepth) {

GdkDrawables have a depth even without a GdkColormap, so you can just

        switch (gdk_drawable_get_depth(aDrawable)) {

Rest looks good.

BTW, a visual for PictFormat lookup would mean that NativeThemes on displays with a RGB24 default visual wouldn't suffer with mozilla.widget.force-24bpp.

http://marc.info/?l=cairo&m=121624409609428&w=2
(In reply to comment #11)
> BTW, a visual for PictFormat lookup would mean that NativeThemes on displays
> with a RGB24 default visual wouldn't suffer with mozilla.widget.force-24bpp.

Vlad pointed out that always passing the GdkDrawable in as the
first arg to gdk_pixmap_new avoids that problem.
Checked patch in -- going to tentatively mark this as fixed...
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 466870
You need to log in before you can comment on or make changes to this bug.