77953 - nsHTMLAnchorElement::GetHrefCString()

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P3)

Description

[Chris Waterson]

This is a definite hotspot that comes up while profiling long pages (cf. bug
56854, accounts for almost half of the time spent in style resolution, 6% of the
overall page load time!). We need some creating thinking on this one. Can we do
this stuff even more lazily than we do now (e.g., in a second pass, after the
page has been laid out?) Logging a task to get creative.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 77090 should help this.  Pulling the construction of SelectorMatchesData out
yet another level (at the cost of some modularity) would help even more. 
(Perhaps they could be arena-allocated as well.)

Another thing that could help with this is shared buffers.  (There are some
other hotspots in style resolution that could use such help.  I'd like to
discuss this with scc sometime...)

Comment 2

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 3

[Boris Zbarsky [:bzbarsky]]

I just tried that bug 56854 with a current build.  Here's what I see

Total hit count: 467
  54 under RuleProcessorData::RuleProcessorData (called from ResolveStyleFor and
     ProbePseudoStyleFor).  Of those 
    44 nsStyleUtil::IsHTMLLink.  Of those
      31 under nsHTMLAnchorElement::GetHrefURI
       5 under nsCOMPtr_base::~nsCOMPtr_base
       4 under nsWebShell::GetLinkState
       3 under nsCOMPtr_base::assign_from_helper
         (the nsCOMPtr stuff seems to be for the result of GetHrefURI)

Some of those hits under GetLinkState could be gotten rid of if history didn't
take an nsCAutoString, call .get() on it, and pass the char* to a function which
calls PL_strlen on the pointer....

Of the 31 hits under nsHTMLAnchorElement::GetHrefURI
  1 is under nsGenericHTMLElement::GetAttr
 30 are under nsContentUtils::NewURIWithDocumentCharset.  Of these
    2 are inside a UCS2-to-UTF8 conversion
   28 are under nsIOService::NewURI.  Of these
     20 are under nsStandardURL::Init.  The rest are extracting schemes,
        constructing nsStandardURL objects, calling Resolve/SetSpec on them,
        etc.

So overall, nsIOService::NewURI is aroun 7% of that pageload and 25% of the
style resolution time....

I'll attach the full profile.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile

Comment 5

[Alec Flett]

Interesting.. now that strings are flat, we can probably do some cleanup in that
history stuff...

where in nsGlobalHistory are we doing this .get()ing?

Comment 6

[Alec Flett]

Interesting.. now that strings are flat, we can probably do some cleanup in that
history stuff...

where in nsGlobalHistory are we doing this .get()ing?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Right before the call to FindRow() in IsVisited().

Comment 8

[Alec Flett]

ok, so two thoughts there:
1) I can fix nsGlobalHistory::FindRow to take either a flat string or a const
char* with a length. Either one is fine. In the mean time, I'm wondering if
nsCAutoString is big enough to handle long URLs. I mean, if we're looking up a
bunch of 100 character URLs, we're going to overflow nsCAutoString every time.
Do we have an nsStackString class yet?

2) do we want some kind of callback mechanism to queue up a bunch of urls to
look up so that style resolution can continue? This sounds like overkill to me.
I think mork is fairly fast for these lookups and it would only give wierd
effects as links are suddenly marked visited after the page is laid out.

But I guess one thing I'm curious about is if we shortcut visited-link lookup if
there is a global style that overrides the difference between :visited and
normal. I mean, a lot of sites use images without borders for links, or say (in
CSS) that all links should look the same, visited or not. do we know if the
style system handles these cases?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I think mork is fairly fast for these lookups

Yeah, see the profile.  The strlen seems to be about 50% of the global history
lookup times.... (which are really minor compared to the other stuff).

> if there is a global style that overrides the difference between :visited and
> normal

There is no really good way to detect this.  So no.

Comment 10

[Alec Flett]

I don't mean a proactive search to avoid this, but are css resolution structures
(i.e. style contexts and the like) smart enough to ONLY ask if we're visited
when we're resolving a :visited selector?

Comment 11

[Boris Zbarsky [:bzbarsky]]

At the moment, no.  But we _always_ have a :visited selector (in the preference
stylesheet) and the link state is cached....  so making that lazy wouldn't help.

Comment 12

[Darin Fisher]

(In reply to comment #6)
> Interesting.. now that strings are flat, we can probably do some cleanup in that
> history stuff...
> 
> where in nsGlobalHistory are we doing this .get()ing?

strings aren't exactly "flat" .. they are single-fragment, but not necessarily
null-terminated.  so, PromiseFlatCString may still copy.  there is no .get()
method on nsAString.

Comment 13

[Darin Fisher]

(In reply to comment #8)
> Do we have an nsStackString class yet?

no, but see nsFixedString.  it can be initialized to use an arbitrary length
PRUnichar buffer that you can allocate on the stack.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Seems like the function doesn't exist anymore; do we want to keep this bug open?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, it can rest in peace.