1092320 - Log a warning to the console when will-change is out of budget

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

> Can we make sure to log something to the web console when we choose to dishonor will-change?  That will help web developers to be able to reason about why will-change doesn't give them the performance benefits that they expect.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is definitely worth doing.  Can you do it?

nsContentUtils::ReportToConsole should make it pretty easy.  (See other users.)

Comment 2

[Benoit Girard (:BenWa)]

Yes for sure, just let me wrap up a pending bug. Leaving the ni? as a reminder.

Comment 3

[Benoit Girard (:BenWa)]

I'm considering doing the following:
- Warn up to once per document OR log as we go in and out of budget but can be spamy for some content so I think once would be better.
- Use a message resembling 'Will-change memory consumption is too high. Surface area covers 123456 pixels squared, budget is the document surface area times 3 (12345 pixels squared). Ignoring will-change for document: <URI>'. Thoughts?

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

This is my first time modifying localization strings so please reviews that more carefully.

Comment 5

[Benoit Girard (:BenWa)]

Testing:
1) Go to reddit.com
2) Open the inspector
3) Modify their body, div, dl ... CSS rule to also set: 'will-change: opacity'.

Message:
Will-change memory consumption is too high. Surface area covers 224506193 pixels squared, budget is the document surface area multiplied by 3 (3311700 pixels squared).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8526999 [details] [diff] [review]
patch

>+# LOCALIZATION NOTE (WillChangeBudgetWarning): Do not translate Will-change, %1$S,%2$S,%3$S are numbers.
>+WillChangeBudgetWarning=Will-change memory consumption is too high. Surface area covers %1$S pixels squared, budget is the document surface area multiplied by %2$S (%3$S pixels squared). All occurances of will-change in the document is ignored when overbudget.

Change the comma to a period and capitalize the following "budget".
occurances -> occurrences
overbudget -> over budget
is ignored -> are ignored

I also tend to think you probably want:
pixels squared -> pixels


>+    nsString url;
>+    key->Document()->GetURL(url);

This is unused; remove it.

>+    nsString usageStr;
>+    usageStr.AppendInt(budget.mBudget);
>+
>+    nsString multiplierStr;
>+    multiplierStr.AppendInt(multiplier);
>+
>+    nsString limitStr;
>+    limitStr.AppendInt(budgetLimit);
>+
>+    const char16_t* params[] = { usageStr.get(), multiplierStr.get(), limitStr.get() };
>+    key->Document()->WarnOnceAbout(nsIDocument::eWillChangeBudget, false, params, 4);

Use NS_ARRAY_LENGTH(params) or ArrayLength(params) instead of 4.  (Note that it's 3!)

r=dbaron on the layout parts and the string

transferring review for the dom parts to Kyle... although it would be good to know why you need a new set of values rather than being able to reuse the other set

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch, r=dbaron

Comment 8

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=06e000606808

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #6)
> transferring review for the dom parts to Kyle... although it would be good
> to know why you need a new set of values rather than being able to reuse the
> other set

Yes, what's wrong with the existing function

Comment 10

[Benoit Girard (:BenWa)]

Because what I have 1) Isn't a deprecation warning, 2) Those warnings have 'nearly' exhausted the 64-bit range (57/64) anyways:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDeprecatedOperationList.h
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#10185

If we don't care about 1) and 2) then I can just force it in there.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Log a warning to the console when will-change is out of budget. r=dbaron,khuey

Removed extraneous change to ContainerLayerComposite.cpp, ready to land.

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c395ddeb9e91

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c395ddeb9e91

Comment 14

[Kazumasa Hasegawa (Kazz)]

Found typo in console message.
"Occurences of will-change over the budget will be ignored."
"Occurences" -> "Occurrences"

Comment 15

[Benoit Girard (:BenWa)]

Thanks for the report, it's been fixed in bug 1191412.