Closed Bug 1032880 Opened 6 years ago Closed 6 years ago

40% of b2g mochitest log is made up of "WARNING: Transparent content with displayports can be expensive"

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dholbert, Assigned: botond)

References

Details

Attachments

(1 file, 3 obsolete files)

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?)
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
Can we put this warning behind a pref (default on) and turn it off
during mochitest, reftest etc?
The NS_WARNING generates too much noise in debug version log on TBPL, which makes reading the log difficult.
I'll put the warning behind a pref as Mats suggested.
Assignee: nobody → botond
Attached patch bug1032880.patch (obsolete) — Splinter Review
Attachment #8460538 - Flags: review?(mats)
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 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+
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.)
Attached patch bug1032880.patch (obsolete) — Splinter Review
Updated patch to correct reviewer. Carrying r+.
Attachment #8460538 - Attachment is obsolete: true
Attachment #8461151 - Flags: review+
Keywords: checkin-needed
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.)
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.
NS_WARNING is debug-only, isn't it? That's what I was getting at.
Oh, yeah, I think you are right.
For the record, I typically run debug builds of B2G :)
(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.
Attached patch bug1032880.patch (obsolete) — Splinter Review
Updated patch to add the #ifdef DEBUG. Carrying r+.
Attachment #8461151 - Attachment is obsolete: true
Attachment #8461200 - Flags: review+
(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)
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.
Attached patch bug1032880.patchSplinter Review
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)
https://hg.mozilla.org/mozilla-central/rev/0d04d8ef1fc7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.