Closed Bug 1656114 Opened 4 years ago Closed 4 years ago

Fix the Document::mSubDocuments usage in Document::ReportUseCounters

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Project Flags:
Fission Milestone M6b
Tracking Flags:
Tracking Status
firefox83 --- fixed

People

(Reporter: hiro, Assigned: heycam)

References

Details

Attachments

(5 files)

Bug 1656114 - Part 1: Don't null check Document::mStyleUseCounters.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1656114 - Part 2: Add mozilla::BitSet.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1656114 - Part 3: Switch use counter storage to mozilla::BitSet.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1656114 - Part 4: Accumulate page use counters in the parent process.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1656114 - Part 5: Report use counters in documents that come out of the bfcache.
47 bytes, text/x-phabricator-request
Details | Review

Document::mSubDocuments has sub documents in the same process. So if we need to aggregate the numbers per the top level content document, we need to somehow collect the information via IPC calls or such.

Component: General → DOM: Core & HTML

Cameron, can you please audit this?

Flags: needinfo?(cam)

Right now, use counter reporting is done under Document::Destroy(). In ReportUseCounters, we call into each document in mSubDocuments to ask them to (a) report their own document-level use counters, and (b) propagate the page-level use counters up to the top-level content document. Since the top-level content document is in the middle of being destroyed, we can't just do this asynchronously.

Maybe we can change to a model of subdocuments pro-actively reporting their use counters up to the top-level content document (e.g. off a background task when a use counter is set).

Flags: needinfo?(cam)

(In reply to Cameron McCormack (:heycam) from comment #2)

Right now, use counter reporting is done under Document::Destroy(). In ReportUseCounters, we call into each document in mSubDocuments to ask them to (a) report their own document-level use counters, and (b) propagate the page-level use counters up to the top-level content document. Since the top-level content document is in the middle of being destroyed, we can't just do this asynchronously.

Maybe we can change to a model of subdocuments pro-actively reporting their use counters up to the top-level content document (e.g. off a background task when a use counter is set).

Or maybe subdocuments send them to the parent process and the top-level content document also send them to the parent process, then in the parent process we can sum up them and report the telemetry data there?

Yes that would work too, and be less IPC traffic overall.

Severity: -- → N/A
Priority: -- → P2

Planning to do something like this:

  • Leave reporting of document use counters to be done by each document under Document::Destroy(), as we do now.
  • At some point early on in a document that would report use counters but which is not a top-level content document, we determine which top-level content document we'll propagate our use counters to as page use counters. We record this document using its (outer?) window ID. We then send a message to the parent process telling it that we will be reporting page use counters to count towards the specified window ID at some point. When the parent process receives this message, it increments a counter for the number of sub-document use counters it'll need to wait for.
  • Under Destroy() for any document that would report use counters, we send a message to the parent process with our final use counter data and the window ID. The parent process accumulates the use counter data. If we've received the expected number of messages containing use counter data, we report them via telemetry.

We might also want to keep the current "loop over sub-documents and ask them to propagate use counters" part of ReportUseCounters, replacing that with browsing content descendant traversal and sending them a message, to avoid making the final telemetry wait for documents to be cycle collected.

One downside is that if a content process crashes, then any document in it that would be the beneficiary of propagated page use counters wouldn't get any page use counters reported at all, since the parent process would still be waiting for further messages. We could work around that by having the top-level content document, under its Destroy, send a message to the parent process saying that it expects all use counter messages to be sent soon, and the parent could kick off a timer to report the telemetry if it doesn't receive the remaining data in time. Not clear if it's worth it though.

Emilio (as you were last to tweak the way we accumulate and report use counter data, in bug 1578700) do you forsee any problems with this?

Flags: needinfo?(emilio)

(In reply to Cameron McCormack (:heycam) from comment #5)

Planning to do something like this:

  • Leave reporting of document use counters to be done by each document under Document::Destroy(), as we do now.
  • At some point early on in a document that would report use counters but which is not a top-level content document, we determine which top-level content document we'll propagate our use counters to as page use counters. We record this document using its (outer?) window ID.

I think you need the (browsing-context-id, top-level-inner-window-id) pair... The outer window can be shared across top level documents.

Emilio (as you were last to tweak the way we accumulate and report use counter data, in bug 1578700) do you forsee any problems with this?

Other than that, no, this sounds like a sensible solution.

Flags: needinfo?(emilio)

Fission M6b because we want use counters to work for our Fission Nightly experiment.

Fission Milestone: --- → M6b
Summary: Audit the Document::mSubDocuments usage in Document::ReportUseCounters → Fix the Document::mSubDocuments usage in Document::ReportUseCounters

Based on comment 5, it looks like Cameron will be working on this.

Assignee: nobody → cam
Severity: N/A → S3
Status: NEW → ASSIGNED
Type: task → defect

Fission dependency already indirectly present via the audit meta bug.

No longer blocks: fission

Adding this to be used when the bit set needs to be sent across IPC.

This changes the way we deal with page use counters so that we can
handle out of process iframes.

Currently, when a parent document is being destroyed, we poke into all
of the sub-documents to merge their use counters into the parent's page
use counters, which we then report via Telemetry. With Fission enabled,
the sub-documents may be out of process. We can't simply turn these
into async IPC calls, since the parent process will be destroyed
shortly.

So instead, each document during its initialization identifies which
ancestor document it will contribute its page use counters to, and
stores its WindowContext Id to identify that ancestor. A message is
sent to the parent process to notify it that page use counter data will
be sent at some later point. That later point is when the document is
being destroyed. It doesn't matter if the ancestor document has already
been destroyed at this point, since all we need is its WindowContext Id
to uniquely identify it. Once the parent process has received all of
the use counters it expects to accumulate to a given WindowContext Id,
it reports them via Telemetry.

Reporting of document use counters remains unchanged and is done by each
document in their content process.

This patch fixes a few other issues:

  • We no longer report use counters for initial about:blank pages, which
    have been having the effect of lowering the document use counter
    percentages, since CONTENT_DOCUMENTS_DESTROYED was being incremented
    for them.

  • We no longer report use counters for documents created by addons.

  • Added proper MOZ_LOG logging for use counters instead of printfs.

Whiteboard: last patch in draft, needs finalization

Please provide an estimate date for the patch in the Whiteboard field as M6b is nearing completion.

Flags: needinfo?(cam)
Whiteboard: last patch in draft, needs finalization → last patch in draft, needs finalization, ETA: ?
Whiteboard: last patch in draft, needs finalization, ETA: ?
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e606925d0468
Part 1: Don't null check Document::mStyleUseCounters. r=emilio
https://hg.mozilla.org/integration/autoland/rev/34138216874d
Part 2: Add mozilla::BitSet. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/7d0e7bdf9d6e
Part 3: Switch use counter storage to mozilla::BitSet. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ef64855313bd
Part 4: Accumulate page use counters in the parent process. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/d7dac872984d
Part 5: Report use counters in documents that come out of the bfcache. r=nika
See Also: → 1845779
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: