Wasted work in gfxContext::ClipContainsRect()

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pchang9, Assigned: pchang9)

Tracking

({perf})

Trunk
mozilla26
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 781387 [details] [diff] [review]
Suggested patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130116073211

Steps to reproduce:

The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it.

In method gfxContext::ClipContainsRect() in gfx/thebes/gfxContext.cpp, the loop in line 1232 keeps overriding "lastReset" with "i" when "mStateStack[i].clipWasReset" is true. Therefore, only the last written value is visible out of the loop and all the other writes and iterations are not necessary. The patch iterates from the start of "lastReset" and breaks the first time when "i" is set.

Similar problems also appear in loops at line 2073 and 2184, function gfxContext::PushClipsToDT() and gfxContext::ChangeTransform().
(Assignee)

Updated

5 years ago
Keywords: perf
OS: Windows 7 → All
Version: 18 Branch → Trunk

Updated

5 years ago
Component: Untriaged → Graphics
Product: Firefox → Core
Whiteboard: patch

Updated

5 years ago
Attachment #781387 - Attachment is patch: true
(Assignee)

Comment 2

5 years ago
Created attachment 784375 [details] [diff] [review]
898228.patch
Attachment #784375 - Flags: review?(jmuizelaar)
Attachment #784375 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 784375 [details] [diff] [review]
898228.patch

Review of attachment 784375 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch! I think the current behavior is actually wrong! We're currently catching the first reset instead of the last one. This is a very nice catch, but the right solution is inserting a break in the existing loops without changing the loops itself. I'm actually surprised this hasn't caused any bugs, I suppose we don't often have multiple resets in a clip stack. When we fix this we should keep a careful eye open on whether nothing breaks.
Attachment #784375 - Flags: review?(bas) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 784394 [details] [diff] [review]
898228-1.patch
Attachment #781387 - Attachment is obsolete: true
Attachment #784375 - Attachment is obsolete: true
Attachment #784394 - Flags: review?(bas)
Comment on attachment 784394 [details] [diff] [review]
898228-1.patch

Review of attachment 784394 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a great change!
Attachment #784394 - Flags: review?(bas) → review+
(Assignee)

Comment 6

5 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #5)
> Comment on attachment 784394 [details] [diff] [review]
> 898228-1.patch
> 
> Review of attachment 784394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a great change!

Thanks! Could you be my voucher so that I can test on try servers ?
(Assignee)

Comment 7

5 years ago
Landed on try server:
https://tbpl.mozilla.org/?tree=Try&rev=75b0a15266b3
(Assignee)

Comment 8

5 years ago
Created attachment 788297 [details] [diff] [review]
898228.patch (bug description updated, try testing)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Attachment #784394 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d215826d8637
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.