Closed Bug 897262 Opened 7 years ago Closed 7 years ago

Wasted work in nsBaseScreen::CheckMinimumBrightness()

Categories

(Core :: Widget, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: pchang9, Assigned: pchang9)

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

Attached file patch.diff (obsolete) —
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.
Keywords: perf
Version: 19 Branch → Trunk
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
Attached patch 897262.patch (obsolete) — Splinter Review
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+
(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
Assignee: nobody → pchang9
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/50dfd0c5bc36
Status: ASSIGNED → RESOLVED
Closed: 7 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 → ---
Attached patch 897262.patch (obsolete) — Splinter Review
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)
(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.
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?
Attached patch 897262-2.patchSplinter Review
Attachment #787789 - Flags: review?(bgirard)
(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?
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+
Attachment #787165 - Attachment is obsolete: true
Target Milestone: mozilla25 → ---
https://hg.mozilla.org/mozilla-central/rev/08bd150857fb
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.