243234 - Possible optimization to nsStandardURL::Init

Status: NEW

(Core :: Networking, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

At the moment, nsStandardURL::Init has the following logic:

if (spec is absolute) {
  SetSpec(spec);
} else {
  resolve absolute spec with Resolve() and set it with SetSpec;
}

For that else clause, wouldn't it be better to do both at once?  Currently we
take the url segments of the base URI, flatten them into a string, chop off part
based on the the new spec, stick it on the end, etc.  We could start with the
segments for the base and create the segments for ourselves all in one go
instead, no?  It may be a lot less work for common cases like <a href="#foo">.

I realize that we'd be duplicating code, but it may make more sense to implement
Resolve() in terms of NewURI instead of the other way around in this case...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Just to clarify, in profiles SetSpec takes about twice as long as Resolve()
(both only called from Init() here, really) for typical links.  So it seems to
me that we should be able to cut the time creating the URI objects by a factor
of two or so....

Comment 2

[Darin Fisher]

hmm... keep in mind that Resolve does not return a normalized URL spec.  it
isn't normalized until it goes through SetSpec (i.e. BuildNormalizedSpec).  i
figure there are probably things we can do to speed up SetSpec.  can you provide
profile data for SetSpec?  also how big is the impact of this code on
performance?  where does this rear its head?

Comment 3

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #2)
> hmm... keep in mind that Resolve does not return a normalized URL spec.

Ah, ok.  That could make the win a lot less... :(

> can you provide profile data for SetSpec?

Sure thing.  See end of comment.

> also how big is the impact of this code on performance?  where does this rear
> its head?

On a page with lots of links (eg an lxr page) nsStandardURL::Init is around 8%
of pageload...

Breakdown for nsStandardURL::Init (8132 total hits):

 4479 nsStandardURL::SetSpec(nsACString const&)
 2208 nsStandardURL::Resolve(nsACString const&, nsACString&)
  658 __restore_rt
  299 nsCSubstring::Assign(char const*, unsigned)
  171 nsACString::~nsACString()
  104 nsCOMPtr_base::assign_with_AddRef(nsISupports*)
   64 net_ExtractURLScheme(nsACString const&, unsigned*, unsigned*, nsACString*)
   13 nsStandardURL::InvalidateCache(int)
  136 in function itself

Breakdown for the 4567 hits under SetSpec (4479 from Init(), the rest from
__restore_rt):

      2483 nsStandardURL::BuildNormalizedSpec(char const*)
       968 nsStandardURL::ParseURL(char const*)
       907 __restore_rt
        62 nsStandardURL::Clear()
        26 net_FilterURIString(char const*, nsACString&)
        18 nsACString::~nsACString()
        15 nsPromiseFlatCString::Init(nsACString const&)
        88 in function itself

Breakdown for BuildNormalizedSpec (2889 total hits, 406 from __restore_rt)

    717 __restore_rt
    580 nsStandardURL::CoalescePath(netCoalesceFlags, char*)
    580 nsStandardURL::nsSegmentEncoder::EncodeSegmentCount(char const*,
                    nsStandardURL::URLSegment const&, short, nsCString&)
    424 nsCSubstring::SetLength(unsigned)
     91 nsStandardURL::AppendSegmentToBuf(char*, unsigned, char const*,
              nsStandardURL::URLSegment&, nsCString const*)
     41 nsACString::~nsACString()
     26 nsStandardURL::SegmentIs(char const*, nsStandardURL::URLSegment const&,
                                 char const*)
     24 nsStandardURL::AppendToBuf(char*, unsigned, char const*, unsigned)
    406 in function itself

Breakdown for ParseURL (1007 total hits, some from __restore_rt):

    487 nsStandardURL::ParsePath(char const*, unsigned, int)
    216 __restore_rt
    169 .L34
     96 .L47
     39 in function itself

(not much informative under those last two either -- mostly __restore_rt).

I'll attach the profile.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile

Comment 5

[Darin Fisher]

so most of the cost is inside BuildNormalizedSpec.  ParseURL is relatively
cheap.  i don't see much value in trying to save the cost of ParseURL.  instead,
i think we should focus on trying to shortcut BuildNormalizedSpec in the cases
where the spec is already normalized (which is probably very often).  if the
spec is already normalized then we should avoid allocating a copy of it.  we
should try to just assign the input spec to mSpec possibly taking advantage of
string buffer sharing.  when i originally wrote BuildNormalizedSpec, i imagined
writing an IsNormalizedSpec function that could quickly scan through the string
to determine if anything would need to be changed.  perhaps now is the time to
do that?

BuildNormalizedSpec is basically an expensive memcpy in the case where spec is
already normalized.  perhaps there is no way to avoid the scaning cost... or
maybe we could have a private version of Resolve that returns a bitmask
indicating which portions are from the old normalized spec and which are from
the new possibly unnormalized spec.  then we could easily shortcut some of the
checks in BuildNormalizedSpec.

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258