2067000 - Optimize console.clear a little

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Comment 1

[Mayank Bansal]

Attachment: Bug 2067000 - Skip parsing a URI to look for a password the spec cannot hold. r?#necko-reviewers,#dom-core-reviewers

Console and nsScriptError both build an nsIURI out of a script filename on
every message, purely to ask whether it carries a password worth hiding. On
the console.clear() testcase in the bug that parse is 19% of the content
process main thread.

A password only reaches nsIURI through a userinfo component, and every
implementation that can report a non-empty one - nsStandardURL, DefaultURI,
and SubstitutingJARURI, which forwards to its source URL - derives it from a
literal '@'. Scanning for that byte first leaves the sanitizing path
unchanged and skips the parse otherwise.

Both call sites now share NS_GetSanitizedSpecFromSpec rather than spelling
the same check two different ways. That unifies one behaviour difference: on
a spec that has a password which cannot be hidden, Console used to fall back
to the raw spec, and nsScriptError to an empty string. The helper keeps
nsScriptError's behaviour, since emitting a spec known to contain a password
is the one outcome worth avoiding.

Comment 2

[Mayank Bansal]

Attachment: Bug 2067000 - Read the process type once in ConsoleAPIStorage. r?#dom-core-reviewers,nchevobbe

_isDestroyedInnerWindow is a no-op outside the parent process, but it decides
that by reading Services.appinfo.processType, which is a full XPConnect
getter round trip, on every recorded console event. That is 3.3% of the
content process main thread on the testcases in this bug.

The process type cannot change, so hoist it to module scope. This matches
what LoginManager, FirefoxRelay and GuardianClient already do.

Comment 3

[Mayank Bansal]

Attachment: Bug 2067000 - Avoid building an nsIStackFrame for console calls that never read one. r?#dom-core-reviewers,nchevobbe

Every console call wraps its captured stack in a JSStackFrame, which registers
a cycle collector JS holder and adds itself to a per-realm hashtable, then
undoes both when the call data dies. On the testcases in this bug that
constructor and destructor pair is 13% of the content process main thread.

For a page calling console.log() or console.clear() nothing reads the frame:
the wrapper exists only so StackFrameToStackEntry can copy six values out of
it. The consumers that genuinely walk the frame chain are the lazily reified
stacktrace property and the dump function, both gated on
ShouldIncludeStackTrace; LogToMozLog; the worker-side reification; and the
window ID search used for callers that supplied no inner ID. Capture the
JS::SavedFrame directly and only wrap it for those.

GetSavedFrameProperties reads the values with the same principal-based
redaction as the nsIStackFrame getters rather than reimplementing that
choice, so a cross-origin frame reports what it did before.

Comment 4

[Mayank Bansal]

Attachment: Bug 2067000 - Clear the console storage from recordEvent instead of a second XPCOM call. r?#dom-core-reviewers,nchevobbe

console.clear() made two XPConnect round trips into ConsoleAPIStorage, one to
clear the window's stored events and one to record the clear event itself.
The marshalling for those dispatches is around 4% of the content process main
thread on the console.clear() testcase in this bug.

recordEvent already receives the event, and the event carries its own level,
so let it drop the stored events itself. Console.cpp and the other two
recordEvent callers are unaffected: ConsoleUtils only emits log, warn and
error, and Console.sys.mjs implements clear as a no-op.

Comment 5

[Mayank Bansal]

profile of the testcase in bug ZZZ : https://share.firefox.dev/3UmSWlZ (2.2s)
mach try auto: https://treeherder.mozilla.org/jobs?repo=try&revision=6aa4d7883b7e6f3116a12900bbbeee8acb602870

Comment 6

[Mayank Bansal]

opt baseline: https://share.firefox.dev/3T43iqA (4.6s)
opt patch: https://share.firefox.dev/4gvkPRy (2.2s)
try: https://treeherder.mozilla.org/jobs?repo=try&revision=2945830b2cdab8aca1afbd7ae06701ce793315d7

Comment 7

[Phabricator Automation]

Comment on attachment 9631486 [details]
Bug 2067000 - Skip parsing a URI to look for a password the spec cannot hold. r?#necko-reviewers,#dom-core-reviewers

Revision D321766 was moved to bug 2068156. Setting attachment 9631486 [details] to obsolete.

Comment 8

[Phabricator Automation]

Comment on attachment 9631487 [details]
Bug 2067000 - Read the process type once in ConsoleAPIStorage. r?#dom-core-reviewers,nchevobbe

Revision D321767 was moved to bug 2068157. Setting attachment 9631487 [details] to obsolete.

Comment 9

[Mayank Bansal]

With Nightly: https://share.firefox.dev/3St9zMl (2.2s)
This is the same as what I got with all the patches applied in comment 6. Which means that the impact of the remaining patches is marginal. I will abandon these.

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for checking! If this doesn't help the performance much, sounds fine to close the bug indeed.