Closed Bug 1092320 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 2 obsolete files)

> 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.
Blocks: will-change
This is definitely worth doing.  Can you do it?

nsContentUtils::ReportToConsole should make it pretty easy.  (See other users.)
Flags: needinfo?(bgirard)
Yes for sure, just let me wrap up a pending bug. Leaving the ni? as a reminder.
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?
Flags: needinfo?(bgirard)
Attached patch patch (obsolete) — Splinter Review
This is my first time modifying localization strings so please reviews that more carefully.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8526999 - Flags: review?(dbaron)
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 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
Attachment #8526999 - Flags: review?(khuey)
Attachment #8526999 - Flags: review?(dbaron)
Attachment #8526999 - Flags: review+
Attached patch patch, r=dbaron (obsolete) — Splinter Review
Attachment #8526999 - Attachment is obsolete: true
Attachment #8526999 - Flags: review?(khuey)
Attachment #8527135 - Flags: review?(khuey)
(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
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
Removed extraneous change to ContainerLayerComposite.cpp, ready to land.
Attachment #8527135 - Attachment is obsolete: true
Attachment #8534630 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c395ddeb9e91
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/c395ddeb9e91
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Found typo in console message.
"Occurences of will-change over the budget will be ignored."
"Occurences" -> "Occurrences"
Thanks for the report, it's been fixed in bug 1191412.
You need to log in before you can comment on or make changes to this bug.