1450783 - Strange spike in NS_NewURI calls in recent BHR data

Status: RESOLVED WONTFIX

(Core :: Networking, defect, P3)

Description

[Alex Thayer [:alexical] (she/her)]

See https://arewesmoothyet.com/?category=content_script&durationSpec=128_512&mode=explore&payloadID=6c97042790a24335a84893b9935c718b&platform=Windows_NT%3A10%3Ax86-64&search=NS_NewURI&thread=0 (Make sure to click on "Content" on the left side of the top section - there seems to be a bug right now in permalinking.)

There's a strange spike I'm seeing in NS_NewURI calls in the content process. The stacks are roughly this:

mozilla::net::nsIOService::NewURI(nsTSubstring<char> const &,char const *,nsIURI *,nsIURI * *)
NS_NewURI(nsIURI * *,nsTSubstring<char> const &,char const *,nsIURI *,nsIIOService *)
NS_NewURI(nsIURI * *,nsTSubstring<char> const &,mozilla::NotNull<mozilla::Encoding const *>,nsIURI *,nsIIOService *)
NS_NewURI(nsIURI * *,nsTSubstring<char16_t> const &,mozilla::NotNull<mozilla::Encoding const *>,nsIURI *,nsIIOService *)
nsContentUtils::NewURIWithDocumentCharset(nsIURI * *,nsTSubstring<char16_t> const &,nsIDocument *,nsIURI *)
nsGenericHTMLElement::GetURIAttr(nsAtom *,nsAtom *,nsIURI * *)
nsGenericHTMLElement::GetURIAttr(nsAtom *,nsAtom *,nsTSubstring<char16_t> &)
static bool mozilla::dom::HTMLScriptElementBinding::get_src(struct JSContext *, class JS::Handle<JSObject *>, class mozilla::dom::HTMLScriptElement *, class JSJitGetterCallArgs)
(unresolved)
(content script)
EventDispatcher::Dispatch
XRE_InitChildProcess
(root)

I'm not altogether sure where this should go, since it's not altogether clear what the underlying cause is. But I figured I'd put the bug up and then continue digging to see if I can find anything.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I wonder if this is at all related to the thread-safety work?

Hey Valentin, could bug 1447194 be related to why we're starting to see NS_NewURI frames creep up in our content process main thread hangs?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

This is an interesting find.
I'm not sure I'm reading this correctly, but I'm assuming the number of calls to nsGenericHTMLElement::GetURIAttr hasn't increased, which means it's NS_NewURI which got slower. The reason could indeed be bug 1447194, which made the refcounting for nsStandardURL/nsSimpleURI threadsafe (with slower atomics).
I'm guessing this is an especially hot path, so the slowdown is more obvious.
As to improving this, I think we could invest in some micro-optimizations to BuildNormalizedSpec/net_CoalesceDirs/etc that would at least offset the slowdown introduced by threadsafe refcounting. Thoughts?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Valentin Gosu [:valentin] from comment #2)
> Thoughts?

Sounds like a good idea to me!

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Not working on this right now.

Comment 5

[Andrew Creskey [:acreskey]]

This doesn't look to still be a concern from a quick look at http://queze.net/bhr/test/#row=0&filter=
But I'm very familiar with the BHR tool.

Comment 6

[Florian Quèze [:florian]]

(In reply to Andrew Creskey [:acreskey] from comment #5)

This doesn't look to still be a concern from a quick look at http://queze.net/bhr/test/#row=0&filter=
But I'm very familiar with the BHR tool.

My BHR dashboard (the most recent version is hosted at https://fqueze.github.io/hang-stats/) only shows data from the parent process main thread. Comment 0 says "There's a strange spike I'm seeing in NS_NewURI calls in the content process." so unfortunately I don't think we can use my dashboard to draw conclusions.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

There's a patch to make parsing a bit faster in bug 1849745.
Otherwise we can't revert the threadsafe refcounting changes.