Closed
Bug 842389
Opened 11 years ago
Closed 11 years ago
ProgressiveUpdate returning incorrect value
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: nrc, Assigned: cwiiis)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.84 KB,
patch
|
BenWa
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
See http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicTiledThebesLayer.cpp#438 It looks like we should not be returning false there. The code does not match the comment.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → ncameron
Comment 1•11 years ago
|
||
It does also look wrong to me. Can you confirm Chris?
Reporter | ||
Comment 2•11 years ago
|
||
Attachment #715254 -
Flags: review?(chrislord.net)
Reporter | ||
Comment 3•11 years ago
|
||
try push: https://tbpl.mozilla.org/?tree=Try&rev=c862641f2214
Assignee | ||
Comment 4•11 years ago
|
||
Comment on attachment 715254 [details] [diff] [review] patch Review of attachment 715254 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch, but I don't think the fix is right. ::: gfx/layers/basic/BasicTiledThebesLayer.cpp @@ +435,5 @@ > aScrollOffset, > aResolution, > repeat); > > + isBufferChanged |= repeat; This doesn't look right either though - If there's no repeat, but the buffer is updated once, this will return false? Seems like you could just put isBufferChanged = true after the regionToPaint.IsEmpty() check.
Attachment #715254 -
Flags: review?(chrislord.net) → review-
Reporter | ||
Comment 5•11 years ago
|
||
Attachment #715254 -
Attachment is obsolete: true
Attachment #715610 -
Flags: review?(chrislord.net)
Reporter | ||
Comment 6•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #4) > Comment on attachment 715254 [details] [diff] [review] > patch > > Review of attachment 715254 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice catch, but I don't think the fix is right. > > ::: gfx/layers/basic/BasicTiledThebesLayer.cpp > @@ +435,5 @@ > > aScrollOffset, > > aResolution, > > repeat); > > > > + isBufferChanged |= repeat; > > This doesn't look right either though - If there's no repeat, but the buffer > is updated once, this will return false? Seems like you could just put > isBufferChanged = true after the regionToPaint.IsEmpty() check. Ah, I misunderstood when repeat would be true. And you are right, your suggestion fixes a bug I have been chasing for a week (on and off). I'm not sure why this is more obvious on the refactoring than trunk, but this is a fairly frequent and annoying bug, we should probably push this patch as far as we can.
Comment 7•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch This is good to go.
Attachment #715610 -
Flags: review?(chrislord.net) → review+
Updated•11 years ago
|
tracking-fennec: --- → ?
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
tracking-firefox22:
--- → ?
Assignee | ||
Comment 8•11 years ago
|
||
ftr, I think we should uplift this to beta and aurora - low risk and high gain.
Reporter | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f044f9e981a
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Progressive Update of tiled layers User impact if declined: rendering glitches, flickering Testing completed (on m-c, etc.): testing on m-c and graphics branch Risk to taking this patch (and alternatives if risky): low, this is a localised and small bug fix String or UUID changes made by this patch: none
Attachment #715610 -
Flags: approval-mozilla-beta?
Attachment #715610 -
Flags: approval-mozilla-aurora?
Comment 11•11 years ago
|
||
Tracking since it looks like this is a regression in FF20 caused by bug 783368
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox22:
--- → affected
Keywords: regression
Comment 12•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch Approving low risk uplift, early in FF20 beta cycle.
Attachment #715610 -
Flags: approval-mozilla-beta?
Attachment #715610 -
Flags: approval-mozilla-beta+
Attachment #715610 -
Flags: approval-mozilla-aurora?
Attachment #715610 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/99c93cc91750
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/819c0ebe4f70
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f044f9e981a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Reporter | ||
Comment 16•11 years ago
|
||
We seem to be having Talos problems: Regression: Mozilla-Beta - Robocop Checkerboarding Real User Benchmark - Android 2.2 (Native) - 127% increase ------------------------------------------------------------------------------------------------------------- Previous: avg 4.743 stddev 0.236 of 30 runs up to revision 58ba8dc9013a New : avg 10.752 stddev 0.245 of 5 runs since revision 99c93cc91750 Change : +6.008 (127% / z=25.428) Graph : http://mzl.la/12Nk5wC Changeset range: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=58ba8dc9013a&tochange=99c93cc91750 Changesets: * http://hg.mozilla.org/releases/mozilla-beta/rev/99c93cc91750 : Nicholas Cameron <ncameron@mozilla.com> - Bug 842389 - return the right thing from ProgressiveUpdate; r=BenWa,cwiiis a=lsblakk : http://bugzilla.mozilla.org/show_bug.cgi?id=842389 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=842389 - ProgressiveUpdate returning incorrect value Regression: Mozilla-Beta - Robocop Checkerboarding Benchmark - Android 2.2 (Native) - 2.31e+03% increase -------------------------------------------------------------------------------------------------------- Previous: avg 0.024 stddev 0.016 of 30 runs up to revision 58ba8dc9013a New : avg 0.591 stddev 0.051 of 5 runs since revision 99c93cc91750 Change : +0.566 (2.31e+03% / z=34.503) Graph : http://mzl.la/WQXOZO Changeset range: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=58ba8dc9013a&tochange=99c93cc91750 Changesets: * http://hg.mozilla.org/releases/mozilla-beta/rev/99c93cc91750 : Nicholas Cameron <ncameron@mozilla.com> - Bug 842389 - return the right thing from ProgressiveUpdate; r=BenWa,cwiiis a=lsblakk : http://bugzilla.mozilla.org/show_bug.cgi?id=842389 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=842389 - ProgressiveUpdate returning incorrect value Regression: Mozilla-Beta - Tp5 No Network Row Major MozAfterPaint (Main RSS) - MacOSX 10.6 (rev4) - 2.88% increase ------------------------------------------------------------------------------------------------------------------ Previous: avg 197677933.333 stddev 2287798.806 of 30 runs up to revision 58ba8dc9013a New : avg 203362800.000 stddev 416356.458 of 5 runs since revision 99c93cc91750 Change : +5684866.667 (2.88% / z=2.485) Graph : http://mzl.la/WRfepf Changeset range: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=58ba8dc9013a&tochange=99c93cc91750 Changesets: * http://hg.mozilla.org/releases/mozilla-beta/rev/99c93cc91750 : Nicholas Cameron <ncameron@mozilla.com> - Bug 842389 - return the right thing from ProgressiveUpdate; r=BenWa,cwiiis a=lsblakk : http://bugzilla.mozilla.org/show_bug.cgi?id=842389 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=842389 - ProgressiveUpdate returning incorrect value
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Nick Cameron [:nrc] from comment #16) > We seem to be having Talos problems: > Regression: Mozilla-Beta - Robocop Checkerboarding Real User Benchmark - > Android 2.2 (Native) - 127% increase > Previous: avg 4.743 stddev 0.236 of 30 runs up to revision 58ba8dc9013a > New : avg 10.752 stddev 0.245 of 5 runs since revision 99c93cc91750 > Change : +6.008 (127% / z=25.428) > Graph : http://mzl.la/12Nk5wC > Regression: Mozilla-Beta - Robocop Checkerboarding Benchmark - Android 2.2 > (Native) - 2.31e+03% increase > Previous: avg 0.024 stddev 0.016 of 30 runs up to revision 58ba8dc9013a > New : avg 0.591 stddev 0.051 of 5 runs since revision 99c93cc91750 > Change : +0.566 (2.31e+03% / z=34.503) > Graph : http://mzl.la/WQXOZO > Regression: Mozilla-Beta - Tp5 No Network Row Major MozAfterPaint (Main RSS) > - MacOSX 10.6 (rev4) - 2.88% increase > Previous: avg 197677933.333 stddev 2287798.806 of 30 runs up to revision > 58ba8dc9013a > New : avg 203362800.000 stddev 416356.458 of 5 runs since revision > 99c93cc91750 > Change : +5684866.667 (2.88% / z=2.485) > Graph : http://mzl.la/WRfepf I would expect drops, we're going to end up doing more work than before (correctly so) - none of these drops look hugely significant though tbh, 127% isn't much when the initial value is so small.
Comment 18•11 years ago
|
||
I think we should tolerate this regression since correctness wins over perf. I will have many more ideas for improving the performance so I rather spend efforts on that instead.
Comment 19•11 years ago
|
||
This will need to be backed out of mozilla-beta (FF20) due to breaking sites (see bug 843997)
Comment 20•11 years ago
|
||
Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/bb28f76aa704
Comment 21•11 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #20) > Backout: > https://hg.mozilla.org/releases/mozilla-beta/rev/bb28f76aa704 Please backout on aurora as well.
Reporter | ||
Comment 24•11 years ago
|
||
Sorry for the bustage here. It would be good to know why this breaks for certain websites - the few I tested with were fine. It is a little worrying that we can render essentially nothing for a whole bunch of sites and still pass all our tests. Cwiiis or BenWa - do you have cycles to look at this or should I (I am a little snowed under with layers refactoring, but could find time for this)?
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Nick Cameron [:nrc] from comment #24) > Sorry for the bustage here. It would be good to know why this breaks for > certain websites - the few I tested with were fine. It is a little worrying > that we can render essentially nothing for a whole bunch of sites and still > pass all our tests. > > Cwiiis or BenWa - do you have cycles to look at this or should I (I am a > little snowed under with layers refactoring, but could find time for this)? I have no idea why this would break - did it break on all versions, or just beta and/or aurora? It doesn't make a whole lot of sense, unless it triggers a path that is otherwise not used (thinking of the broken single-tile path) I can look at this next week, though I may not get to it until mid-week, depending on how bug 716403 goes. Would that be ok?
Reporter | ||
Comment 26•11 years ago
|
||
I tried to investigate this on the graphics branch where the patch is still applied, but I could not reproduce, which is a bit odd. I'll try with trunk once I get a build going.
Reporter | ||
Comment 27•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #25) > (In reply to Nick Cameron [:nrc] from comment #24) > > Sorry for the bustage here. It would be good to know why this breaks for > > certain websites - the few I tested with were fine. It is a little worrying > > that we can render essentially nothing for a whole bunch of sites and still > > pass all our tests. > > > > Cwiiis or BenWa - do you have cycles to look at this or should I (I am a > > little snowed under with layers refactoring, but could find time for this)? > > I have no idea why this would break - did it break on all versions, or just > beta and/or aurora? It doesn't make a whole lot of sense, unless it triggers > a path that is otherwise not used (thinking of the broken single-tile path) > > I can look at this next week, though I may not get to it until mid-week, > depending on how bug 716403 goes. Would that be ok? That'd be great, thanks! I'm vaguely looking into it, so let me know when you start. I'll post anything I find here.
Reporter | ||
Comment 28•11 years ago
|
||
Well I couldn't recreate the bug, all the mobile sites rendered fine for me with the patch. Over to you :-) Let me know if I can help out some how.
Updated•11 years ago
|
Whiteboard: [no-nag]
Updated•11 years ago
|
Whiteboard: [no-nag]
Comment 29•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch Marking this patch obsolete so we can continue to monitor this patch for tracking without sending "unlanded" email reminders daily.
Attachment #715610 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Assignee: ncameron → chrislord.net
Comment 30•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #25) > I can look at this next week, though I may not get to it until mid-week, > depending on how bug 716403 goes. Would that be ok? Hey Chris - since this bug is a tracked regression and bug 716403 is not currently tracking for a particular release (that I can see) do you have time to try another fix here that we could get landed to trunk, verified and hopefully uplifted before Tues March 12th when we go to build with FF20 beta 4?
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #25) > I have no idea why this would break - did it break on all versions, or just > beta and/or aurora? I could still do with an answer to this - seeing as Nick couldn't reproduce, did this break in all versions or just non-Nightly? Do we have some solid STR?
Flags: needinfo?
Comment 32•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch clearing the approvals since this got backed out
Attachment #715610 -
Flags: approval-mozilla-beta+
Attachment #715610 -
Flags: approval-mozilla-aurora+
Flags: needinfo?
Assignee | ||
Comment 33•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch Removing the obsolete flag seeing as the approval has been removed - I still see nothing wrong with this patch and suspect it's a bad interaction with old code. I'm going to test this on Nightly and if I can't find anything, I'm pushing it to inbound - if there are issues, we need STR, otherwise we can let this ride the trains.
Attachment #715610 -
Attachment is obsolete: false
Assignee | ||
Comment 34•11 years ago
|
||
I also can't reproduce on Nightly, will push when the tree is open.
Assignee | ||
Comment 35•11 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c58cb6c3b86
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9c58cb6c3b86
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 37•11 years ago
|
||
Comment on attachment 715610 [details] [diff] [review] patch [Triage Comment] We need to try uplifting this again and see if the issues from previous attempt reproduce. As Benoit stated in comment 18, if there is just a perf regression we should tolerate it for correctness and to not ship this regression in code.
Attachment #715610 -
Flags: approval-mozilla-beta+
Attachment #715610 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #37) > Comment on attachment 715610 [details] [diff] [review] > patch > > [Triage Comment] > We need to try uplifting this again and see if the issues from previous > attempt reproduce. As Benoit stated in comment 18, if there is just a perf > regression we should tolerate it for correctness and to not ship this > regression in code. I think these issues were caused by bug 850396, which I've marked as blocking this - it's a pretty harmless change - it will be a performance regression, possibly, but using that path will very often result in rendering errors anyway.
Comment 39•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e414cc5ca5a https://hg.mozilla.org/releases/mozilla-beta/rev/06611d201df5
Assignee | ||
Comment 40•11 years ago
|
||
hmm, we really don't want to have this pushed without the patch on bug 850396.. Is that also being uplifted? Because if not, this *will* cause problems.
Updated•11 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•