Closed
Bug 1032880
Opened 10 years ago
Closed 10 years ago
40% of b2g mochitest log is made up of "WARNING: Transparent content with displayports can be expensive"
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: dholbert, Assigned: botond)
References
Details
Attachments
(1 file, 3 obsolete files)
4.99 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
While poking at bug 1032782, I noticed that a single warning makes up an inordinate proportion of the b2g_emulator_vm mochitest-debug-15 log. This warning in particular: { 01:14:59 INFO - [Child 725] WARNING: Transparent content with displayports can be expensive.: file ../../../gecko/layout/base/nsDisplayList.cpp, line 1371 } Sample log: https://tbpl.mozilla.org/php/getParsedLog.php?id=42836357&tree=Mozilla-Inbound b2g_emulator_vm mozilla-inbound debug test mochitest-debug-15 on 2014-07-01 00:24:14 PDT for push ac9475dcc406 The full log (downloaded and gunzipped) is 504K. I did a search-and-replace to strip out all instances of this warning, and that left the log at 308K. So, nearly 40% of this log is occupied by instances of just this one warning message. CC'ing botond, who touched this warning most recently, in the hopes that he might know the significance of this. (Should we be worried? Should we silence the warning?)
Reporter | ||
Comment 1•10 years ago
|
||
Archeology: This NS_WARNING goes back to bug 693938, whose patch (mozilla-central changeset 969648d51825) added it with some slightly-different language. ("We don't support transparent content with displayports, force it to be opqaue") The language was updated to its current form in bug 964517 (mozilla-central changeset 7ddabb9fd909). Looks like that patch was originally going to remove the warning entirely, but roc requested (in bug 964517 comment 44) that we keep it, because it helps us identify apps that are taking a slow path. It was noted in subsequent comments that Gaia apps, Fennec, & unit tests do trigger this warning; so, the current spamminess described in comment 0 here is probably not due to any recent change.
Depends on: 964517
Comment 2•10 years ago
|
||
Can we put this warning behind a pref (default on) and turn it off during mochitest, reftest etc?
Comment 3•10 years ago
|
||
The NS_WARNING generates too much noise in debug version log on TBPL, which makes reading the log difficult.
Assignee | ||
Comment 4•10 years ago
|
||
I'll put the warning behind a pref as Mats suggested.
Assignee: nobody → botond
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8460538 -
Flags: review?(mats)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8460538 [details] [diff] [review] bug1032880.patch Review of attachment 8460538 [details] [diff] [review]: ----------------------------------------------------------------- Just realized Mats is on vacation, changing reviewer to Timothy.
Attachment #8460538 -
Flags: review?(mats) → review?(tnikkel)
Comment 7•10 years ago
|
||
Comment on attachment 8460538 [details] [diff] [review] bug1032880.patch It'd be nice if we had a unified way to enable warnings when content is using something that hits a perf cliff like this, and like http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4010 but this is good enough.
Attachment #8460538 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 8•10 years ago
|
||
green try |
Try push: https://tbpl.mozilla.org/?tree=Try&rev=11e12337725f (I'm not taking any chances after I landed what I thought was a trivial change without a Try push yesterday, only to have it backed out.)
Assignee | ||
Comment 9•10 years ago
|
||
Updated patch to correct reviewer. Carrying r+.
Attachment #8460538 -
Attachment is obsolete: true
Attachment #8461151 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8461151 [details] [diff] [review] bug1032880.patch >diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp > if (usingDisplayport && >- !(root->GetContentFlags() & Layer::CONTENT_OPAQUE)) { >+ !(root->GetContentFlags() & Layer::CONTENT_OPAQUE) && >+ SpammyLayoutWarningsEnabled()) { > // See bug 693938, attachment 567017 [details] > NS_WARNING("Transparent content with displayports can be expensive."); > } (per IRC, let's also add an #ifdef DEBUG wrapper around this whole "if" clause, so that opt builds don't have to bother with this pref-check for no reason. If you add this in a separate patch, then rs=me on that patch.)
Comment 11•10 years ago
|
||
I don't know if running debug builds is reasonable on b2g, or if a lot of people do it, so then the point of the warning might be lost completely.
Reporter | ||
Comment 12•10 years ago
|
||
NS_WARNING is debug-only, isn't it? That's what I was getting at.
Comment 13•10 years ago
|
||
Oh, yeah, I think you are right.
Assignee | ||
Comment 14•10 years ago
|
||
For the record, I typically run debug builds of B2G :)
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #14) > For the record, I typically run debug builds of B2G :) One of the main reasons being that content, frame, and display list dumps are easier to get this way.
Assignee | ||
Comment 16•10 years ago
|
||
Updated patch to add the #ifdef DEBUG. Carrying r+.
Attachment #8461151 -
Attachment is obsolete: true
Attachment #8461200 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab8ef0895ca
Keywords: checkin-needed
Comment 18•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #15) > (In reply to Botond Ballo [:botond] from comment #14) > > For the record, I typically run debug builds of B2G :) > > One of the main reasons being that content, frame, and display list dumps > are easier to get this way. frame and display list dumps come with enable dump painting builds. content dumps, however, you still need debug for unfortunately.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4119caa3ac8f for this bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=44471729&tree=Mozilla-Inbound > /builds/slave/m-in-l64-000000000000000000000/build/layout/base/nsDisplayList.cpp:1270:8: error: variable 'usingDisplayport' set but not used [-Werror=unused-but-set-variable] > /builds/slave/m-in-l64-000000000000000000000/build/layout/base/nsDisplayList.cpp:69:1: error: 'bool SpammyLayoutWarningsEnabled()' defined but not used [-Werror=unused-function]
Flags: needinfo?(botond)
Reporter | ||
Comment 20•10 years ago
|
||
ah. Those should both be #ifdef DEBUG as well, I think. And perhaps the "usingDisplayport" decl & value-setting-chunk should move down to be adjacent to its usage? i.e. adjacent to the code quoted in comment 10, inside the same #ifdef that's now wrapping that code. (unless it's important that it be before the code that separates those chunks for some reason) This also means we're currently making a useless call to nsLayoutUtils::GetDisplayPort in opt builds, I guess, to initialize this boolean that ends up only guarding a NS_WARNING. Probably not too expensive, but nice to shave off that unnecessary call.
Updated•10 years ago
|
QA Whiteboard: [qa-]
Assignee | ||
Comment 21•10 years ago
|
||
green try |
Updated patch to fix Werrors. Carrying r+. Build-only try push for Linux to make sure there are no further Werrors: https://tbpl.mozilla.org/?tree=Try&rev=299e39bb6154 (My local build is B2G, which has Werror disabled, hence the recurring annoyance of not catching Werrors locally.)
Attachment #8461200 -
Attachment is obsolete: true
Attachment #8461680 -
Flags: review+
Flags: needinfo?(botond)
Assignee | ||
Comment 22•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d04d8ef1fc7
https://hg.mozilla.org/mozilla-central/rev/0d04d8ef1fc7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•