Lots of URI parsing in network predictor

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: u408661)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p3][necko-active])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
See this profile please: https://perfht.ml/2fFRnw3
(Reporter)

Updated

2 years ago
Blocks: 1367339
Whiteboard: [qf:p3]
(Reporter)

Comment 1

2 years ago
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!
Flags: needinfo?(hurley)
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → hurley
Whiteboard: [qf:p3] → [qf:p3][necko-active]
(Reporter)

Comment 3

2 years ago
Thanks a lot!
(Assignee)

Comment 4

2 years ago
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.
Flags: needinfo?(hurley)
Comment hidden (mozreview-request)
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?
Attachment #8900858 - Flags: review?(valentin.gosu) → review-
(Assignee)

Comment 8

2 years ago
(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 hidden (mozreview-request)
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!
Attachment #8900858 - Flags: review?(valentin.gosu) → review+

Comment 11

2 years ago
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/730e52884364
only parse URIs in the predictor when absolutely necessary. r=valentin
https://hg.mozilla.org/mozilla-central/rev/730e52884364
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.