Wasted work in nsBaseScreen::CheckMinimumBrightness()

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pchang9, Assigned: pchang9)

Tracking

({perf})

Trunk
mozilla26
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 780057 [details]
patch.diff

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0 (Beta/Release)
Build ID: 20130316161634

Steps to reproduce:

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

In method nsBaseScreen::CheckMinimumBrightness() in widget/xpwidgets/nsBaseScreen.cpp, the loop in line 70 keeps overriding "brightness" with "i" when "mBrightnessLocks[i]" is greater than zero. 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 end of "i" and breaks the first time when "brightness" is set.
(Assignee)

Updated

5 years ago
Keywords: perf
Version: 19 Branch → Trunk
(Assignee)

Updated

5 years ago
Attachment #780057 - Flags: review?(pchang9)
Hey, thanks for the patch! To request review, you need to put someone else's name in the "requestee" field. See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed for more detail.
Component: Untriaged → Widget
Product: Firefox → Core
(Assignee)

Comment 2

5 years ago
Created attachment 784380 [details] [diff] [review]
897262.patch
Attachment #784380 - Flags: review?(bgirard)
Comment on attachment 784380 [details] [diff] [review]
897262.patch

I don't see any functional change. This is a micro-optimization right? Tentatively r+ on the answer being yes.
Attachment #784380 - Flags: review?(bgirard) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 784380 [details] [diff] [review]
> 897262.patch
> 
> I don't see any functional change. This is a micro-optimization right?
> Tentatively r+ on the answer being yes.

Thanks for your review. Yes, it is a micro-optimization.
Cool. Thanks for the patch! Let me know if you need help finding something else to work on. This should land to our integration branch in a few hours and be merged to the central branch in ~24 hours.
Keywords: checkin-needed

Updated

5 years ago
Assignee: nobody → pchang9
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/50dfd0c5bc36
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
I can see a behaviour change. If no mBrightnessLocks[i] are greater than zero, this is an infinite loop, as i >= 0 is always true for an unsigned integer.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

5 years ago
Created attachment 787165 [details] [diff] [review]
897262.patch
Attachment #780057 - Attachment is obsolete: true
Attachment #784380 - Attachment is obsolete: true
Attachment #780057 - Flags: review?(pchang9)
Attachment #787165 - Flags: review?(bgirard)
Attachment #787165 - Flags: review?(Ms2ger)
(Assignee)

Comment 10

5 years ago
(In reply to :Ms2ger from comment #8)
> I can see a behaviour change. If no mBrightnessLocks[i] are greater than
> zero, this is an infinite loop, as i >= 0 is always true for an unsigned
> integer.

Thanks for pointing out the problem! I have update the patch, please review it.

Updated

5 years ago
Attachment #787165 - Flags: review?(bgirard)
Attachment #787165 - Flags: review?(Ms2ger)
Attachment #787165 - Flags: review+
Looks good but the patch no longer applies to mozilla-central because your first patch has landed. Can you attach a 'rebased' version of the patch?
(Assignee)

Comment 12

5 years ago
Created attachment 787789 [details] [diff] [review]
897262-2.patch
Attachment #787789 - Flags: review?(bgirard)
(Assignee)

Comment 13

5 years ago
(In reply to Benoit Girard (:BenWa) from comment #11)
> Looks good but the patch no longer applies to mozilla-central because your
> first patch has landed. Can you attach a 'rebased' version of the patch?

Sure, please review 897262-2.patch. Do I have to say more in patch description?
(Assignee)

Comment 14

5 years ago
Landed on try server
https://tbpl.mozilla.org/?tree=Try&rev=bb88895a4186
Comment on attachment 787789 [details] [diff] [review]
897262-2.patch

A better description for this change would be 'Bug 897262 - Avoid infinite loop in nsBaseScreen::CheckMinimumBrightness(). r=bgirard.' but I think it's fine as-is since it's a small fix.
Attachment #787789 - Flags: review?(bgirard) → review+

Updated

5 years ago
Attachment #787165 - Attachment is obsolete: true

Updated

5 years ago
Keywords: checkin-needed
Target Milestone: mozilla25 → ---
https://hg.mozilla.org/mozilla-central/rev/08bd150857fb
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.