1390274 - Lots of URI parsing in network predictor

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[(no longer active)]

See this profile please: https://perfht.ml/2fFRnw3

Comment 1

[(no longer active)]

Apologies for filing the bug in a rush.

I'm not too sure about what to do here, if anything.  I'd appreciate if someone familiar with this code can take a look to see if there is a way to avoid some of the URI parsing.  Nick, IIRC you were familiar with the network predictor, do you mind taking a quick look please?  Note that I didn't spend too much time when filing this bug here, so it's possible that the bug is completely invalid, for now please take this more of a "maybe there's something interesting for someone to look at" than anything else.  :-)  Thanks!

Comment 2

[u408661]

Thanks for filing this, Ehsan. I remember doing some work a while back around URI parsing in the predictor to reduce the amount of work being done there - don't have the bug number handy, though I seem to remember you being involved in it! :) (We got some pretty good perf wins there, too.) I'm not sure how much more I can reduce the amount of work, it will probably take getting more creative than last time if I'm to have any success.

I'll take a look at this soon. Leaving ni? so it doesn't get too far down my todo list.

Comment 3

[(no longer active)]

Thanks a lot!

Comment 4

[u408661]

Quick update in case anyone's following along at home: looks like I can cut out most (but not all) of the URI parsing done by the predictor, and 100% of the uri parsing done in the code path that's showing up in the profile. Working up a patch now.

Comment 5

[u408661]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f78849592f19da4aa57d3147ef52542d6be06be

Comment 6

[u408661]

Attachment: Bug 1390274 - only parse URIs in the predictor when absolutely necessary.

Review commit: https://reviewboard.mozilla.org/r/172302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172302/

Comment 7

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

Comment on attachment 8900858 [details]
Bug 1390274 - only parse URIs in the predictor when absolutely necessary.

https://reviewboard.mozilla.org/r/172302/#review177582

::: commit-message-a9d37:1
(Diff revision 1)
> +Bug 1390274 - only parse URIs in the predictor when absolutely necessary. r?valentin

Can you add some more details as to how this patch fixes the problem?

From what I can tell, the URL parsing is moved from ParseMetaDataEntry to SetupPrediction.
Does this improve performance because ParseMetaDataEntry also gets called for other reasons, where a parsed URI is not always necessary?
A more details commit message or comments in the code would be very helpful.

::: netwerk/base/Predictor.cpp:1378
(Diff revision 1)
> -    mPreconnects.AppendElement(uri);
> +    nsCOMPtr<nsIURI> preconnectURI;
> +    rv = NS_NewURI(getter_AddRefs(preconnectURI), uri, nullptr, nullptr,
> +                   mIOService);
> +    if (NS_SUCCEEDED(rv)) {
> +      mPreconnects.AppendElement(preconnectURI);
> +    }

nit: this code seems to be repeating 3 times without much variation.
Could we just bring the NewURI bit outside the if-else block?

Comment 8

[u408661]

(In reply to Valentin Gosu [:valentin] from comment #7)
> Comment on attachment 8900858 [details]
> Bug 1390274 - only parse URIs in the predictor when absolutely necessary.
> 
> https://reviewboard.mozilla.org/r/172302/#review177582
> 
> ::: commit-message-a9d37:1
> (Diff revision 1)
> > +Bug 1390274 - only parse URIs in the predictor when absolutely necessary. r?valentin
> 
> Can you add some more details as to how this patch fixes the problem?
> 
> From what I can tell, the URL parsing is moved from ParseMetaDataEntry to
> SetupPrediction.
> Does this improve performance because ParseMetaDataEntry also gets called
> for other reasons, where a parsed URI is not always necessary?
> A more details commit message or comments in the code would be very helpful.

Right, we were parsing the URI in ParseMetaDataEntry whether we were going to actually use it later or not. I'll update the commit message. (I had already removed some instances of this, but when writing this patch, I realized I could just get rid of them all, and so removed it entirely.)

> ::: netwerk/base/Predictor.cpp:1378
> (Diff revision 1)
> > -    mPreconnects.AppendElement(uri);
> > +    nsCOMPtr<nsIURI> preconnectURI;
> > +    rv = NS_NewURI(getter_AddRefs(preconnectURI), uri, nullptr, nullptr,
> > +                   mIOService);
> > +    if (NS_SUCCEEDED(rv)) {
> > +      mPreconnects.AppendElement(preconnectURI);
> > +    }
> 
> nit: this code seems to be repeating 3 times without much variation.
> Could we just bring the NewURI bit outside the if-else block?

Yeah, it kinda sucks. But if it's done outside the if-else block, we'll still end up parsing a bunch of URIs we never use. I could macro-fy (or static-inline-ify) this to reduce duplciated lines, but that doesn't seem all that productive.

Comment 9

[u408661]

Comment on attachment 8900858 [details]
Bug 1390274 - only parse URIs in the predictor when absolutely necessary.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172302/diff/1-2/

Comment 10

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

Comment on attachment 8900858 [details]
Bug 1390274 - only parse URIs in the predictor when absolutely necessary.

https://reviewboard.mozilla.org/r/172302/#review177610

Looks good. Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/730e52884364
only parse URIs in the predictor when absolutely necessary. r=valentin

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/730e52884364