Details

Reviewers

smaug
• aabh
asuth
sparky

Group Reviewers

dom-worker-reviewers
profiler-reviewers
perftest-reviewers

Commits

rMOZILLACENTRAL1910e803da91: Bug 1899749 - Use profiler markers for getting performance data r=dom-worker…
rMOZILLACENTRAL6406154d2855: Bug 1889121 - Use profiler markers for getting performance data r=dom-worker…

Bugzilla Bug ID

1899749

Summary

Use profiler markers to collect timing data. Markers of known name:

AUTO_PROFILER_MARKER_TEXT("interesting thing #1", DOM, {}, ""_ns);
AUTO_PROFILER_MARKER_TEXT("interesting thing #2", DOM, {}, ""_ns);

can be inspected from the perftest:

await startProfiler();
interestingThings();
let pdata = await stopProfiler();
let duration_ms = inspectProfile(pdata, [
    "interesting thing #1",
    "interesting thing #2"
]);

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Build Status

Buildable 687047
Build 786372: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

jmarshall requested review of this revision.Jun 3 2024, 11:30 PM

jmarshall updated this revision to Diff 871203.

Harbormaster completed remote builds in B667559: Diff 871203.Jun 3 2024, 11:30 PM

smaug requested changes to this revision.Jun 4 2024, 8:47 AM

smaug added a subscriber: smaug.

smaug added inline comments.

dom/serviceworkers/ServiceWorkerActors.cpp
35	"swreg perftest" is quite unusual text for a profiler marker. Do we need that at all? It doesn't seem to add any information to the marker. Marker name (like "InitServiceWorkerRegistrationParent") is enough, and EmptyCString() could be passed as the last param. "swreg perftest" would be shown in all the profiler profiles, not only in the service worker performance tests, and I don't think we want that.

This revision now requires changes to proceed.Jun 4 2024, 8:47 AM

jmarshall added inline comments.Jun 4 2024, 12:02 PM

dom/serviceworkers/ServiceWorkerActors.cpp
35	This is used to identify the markers associated with the test--see line 71 of test_registration.html: duration_ms = inspectProfile(pdata, "swreg perftest"); I think the name and additional text are the only settable parts of markers we can use to identify them.

jmarshall added a reviewer: profiler-reviewers.Jun 4 2024, 12:02 PM

jmarshall requested review of this revision.Jun 4 2024, 2:27 PM

jmarshall updated this revision to Diff 871469.

jmarshall retitled this revision from Bug 1899749 - Use profiler markers for getting performance data r=#dom-worker-reviewers! to Bug 1889121 - Use profiler markers for getting performance data r=#dom-worker-reviewers!.

jmarshall changed the Bugzilla Bug ID from 1899749 to 1889121.

Harbormaster completed remote builds in B667753: Diff 871469.Jun 4 2024, 2:27 PM

jmarshall added a child revision: D212565: WIP: Bug 1889121 - Add service worker update perftests.Jun 4 2024, 2:27 PM

So does this still have "swreg perftest" on purpose?
Looks like https://phabricator.services.mozilla.com/D212565 doesn't use that.

• aabh requested changes to this revision.Jun 4 2024, 5:09 PM

• aabh added a subscriber: • aabh.

• aabh added inline comments.

dom/serviceworkers/ServiceWorkerActors.cpp
35	I would agree with @smaug - the `swreg perftest` data will show up in collected profiles which might be confusing. From a quick search, it looks like the names of the markers introduced in this patch should be enough to uniquely identify them for the tests, no extra text needed. (Please correct me if I'm wrong).
dom/serviceworkers/test/performance/perfutils.js
111	Nit: I presume "Col" is short for "Column". This confused me at first, as we tend to think of these as indices, so a name like `nameIdx` or `nameIx` might be more descriptive.

This revision now requires changes to proceed.Jun 4 2024, 5:09 PM

jmarshall requested review of this revision.Jun 5 2024, 4:00 PM

jmarshall updated this revision to Diff 872220.

Harbormaster completed remote builds in B668307: Diff 872220.Jun 5 2024, 4:00 PM

jmarshall updated this revision to Diff 872222.Jun 5 2024, 4:04 PM

Harbormaster completed remote builds in B668309: Diff 872222.Jun 5 2024, 4:04 PM

In D211368#7296823, @smaug wrote:

So does this still have "swreg perftest" on purpose?
Looks like https://phabricator.services.mozilla.com/D212565 doesn't use that.

Now removed.

rs+, but I think asuth should say whether these markers are the most important ones.
And whether the duration of the markers is what we need to measure, or time between the markers.

dom/serviceworkers/test/performance/perfutils.js
3	en-IN ?
81	Hmm, how large is the profileData? Is it possible that it is in some cases too large for setAsyncMessage?

jmarshall updated this revision to Diff 872722.Jun 6 2024, 12:59 PM

Harbormaster completed remote builds in B668686: Diff 872722.Jun 6 2024, 12:59 PM

jmarshall marked an inline comment as done.Jun 6 2024, 1:01 PM

jmarshall added inline comments.Jun 6 2024, 1:09 PM

dom/serviceworkers/test/performance/perfutils.js
81	If we fail to serialize, the test will fail and not generate perf data, so at least we won't pollute results.

Very cool!

Testing: Performance tests are specifically updated.

• aabh accepted this revision.Jun 10 2024, 7:31 PM

This revision is now accepted and ready to land.Jun 10 2024, 7:31 PM

jmarshall edited the summary of this revision. (Show Details)Jun 10 2024, 7:34 PM

Closed by commit rMOZILLACENTRAL6406154d2855: Bug 1889121 - Use profiler markers for getting performance data r=dom-worker… (authored by jmarshall). · Explain WhyJun 10 2024, 10:39 PM

This revision was automatically updated to reflect the committed changes.

jmarshall added a commit: rMOZILLACENTRAL6406154d2855: Bug 1889121 - Use profiler markers for getting performance data r=dom-worker….

ctuns reopened this revision.Jun 11 2024, 11:05 AM

This revision is now accepted and ready to land.Jun 11 2024, 11:05 AM

ctuns added a reverting change: rMOZILLACENTRALe49ddb0dc83b: Backed out changeset 6406154d2855 (bug 1889121) for causing mochitest failures….Jun 11 2024, 11:05 AM

ctuns added a reverting change: rMOZILLACENTRAL5433a7f0c9cc: Backed out changeset 6406154d2855 (bug 1889121) for causing mochitest failures….Jun 11 2024, 1:37 PM

jmarshall updated this revision to Diff 879579.Jun 20 2024, 12:24 PM

jmarshall retitled this revision from Bug 1889121 - Use profiler markers for getting performance data r=#dom-worker-reviewers! to Bug 1903766 - Use profiler markers for getting performance data r=#dom-worker-reviewers!.

jmarshall edited the summary of this revision. (Show Details)

jmarshall changed the Bugzilla Bug ID from 1889121 to 1903766.

Harbormaster completed remote builds in B674732: Diff 879579.Jun 20 2024, 12:24 PM

jmarshall added a child revision: D214386: WIP: Bug 1903766 - Add service worker update perftests.Jun 20 2024, 12:24 PM

canaltinova added a subscriber: canaltinova.Jul 9 2024, 12:51 PM

canaltinova added inline comments.

dom/serviceworkers/test/performance/perfutils.js
57	Do we really need all the threads? If we know which threads add these markers for sure, it would be better to limit to only those threads to reduce the profiler overhead.

jmarshall retitled this revision from Bug 1903766 - Use profiler markers for getting performance data r=#dom-worker-reviewers! to Bug 1899749 - Use profiler markers for getting performance data r=#dom-worker-reviewers!.Jul 19 2024, 12:32 PM

jmarshall changed the Bugzilla Bug ID from 1903766 to 1899749.

jmarshall updated this revision to Diff 892079.Jul 19 2024, 1:32 PM

jmarshall edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B683997: Diff 892079.Jul 19 2024, 1:32 PM

jmarshall added a child revision: D217093: WIP: Disable profiler integration because of Bug 1885378.Jul 19 2024, 1:32 PM

jmarshall updated this revision to Diff 894553.Jul 25 2024, 2:44 AM

jmarshall edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B685813: Diff 894553.Jul 25 2024, 2:44 AM

jmarshall updated this revision to Diff 894562.Jul 25 2024, 3:07 AM

jmarshall edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B685822: Diff 894562.Jul 25 2024, 3:07 AM

jmarshall added a child revision: D217665: WIP: Bug 1899749 - disable test_registration.html for tsan builds.Jul 25 2024, 3:07 AM

jmarshall updated this revision to Diff 894700.Jul 25 2024, 12:56 PM

jmarshall edited the summary of this revision. (Show Details)

Herald added a reviewer: perftest-reviewers. · View Herald TranscriptJul 25 2024, 12:56 PM

This revision now requires review to proceed.Jul 25 2024, 12:56 PM

Harbormaster completed remote builds in B685929: Diff 894700.Jul 25 2024, 12:56 PM

You've pushed a patch that is making changes in areas of Performance Testing.

Please ensure that all the documentation related to this change is updated. Additionally, please ensure that documentation is added for any new features, workarounds, or enhancements.

You can find the existing documentation for Performance Testing here: https://firefox-source-docs.mozilla.org/testing/perfdocs/

jmarshall updated this revision to Diff 894709.Jul 25 2024, 1:09 PM

Harbormaster completed remote builds in B685935: Diff 894709.Jul 25 2024, 1:09 PM

jmarshall marked an inline comment as done.Jul 25 2024, 1:10 PM

NOTE: A documentation file was modified in diff 894709

It can be previewed for one week: