Closed Bug 29611 Opened 20 years ago Closed 20 years ago

GetLinkState is expensive and it is called for every :link pseudoclass rule

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Other
Other
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: travis, Assigned: attinasi)

References

()

Details

(Keywords: perf, Whiteboard: 8/11: Requested verification by reporter)

Attachments

(1 file)

Going to a page with only one link was calling nsWebShell::GetLinkState for the 
same link 11 times.  GetLinkState is fairly expensive as it has to hit the DB 
etc.  Should we really have to resolve this so many times?
any idea how costly this is?

if this is expensive, it will really hurt our performance on pages like
my.netscape.com, home.netscape.com and www.yahoo.com, which are just links.
I changed GetLinkState to basically do nothing and I got an improvement of .2 
seconds consistently on the reload of a page.  So the improvement should be 
somewhere in between there as it would have to do something at least once.
Assigning to Waqar: 1/3 of Pierre's NEW bugs to help reduce his doomage factor
Assignee: pierre → waqar
Reassigned back to me these bugs that shouldn't have left my list.
Assignee: waqar → pierre
Blocks: 12493
Taking performance related style bugs that are still NEW
Assignee: pierre → attinasi
This is a performance problem. I'll look into it after beta2
Status: NEW → ASSIGNED
Keywords: perf
Target Milestone: --- → M17
Well, I couldn't wait until after beta to look...

GetLinkState is called for every style rule that has a :link, :visited, 
or :out-of-date pseudoclass. I removed all but the basic ones from html.css and 
we get down to three calls (one for each of the three link-pseudos, using a 
testpage with one link). html.css has loads of these pseudos in it, it looks 
like they are for images as links, and for focus on links. I think that we have 
to cache the linkstate if we want to reduce the cost of resolving these 
pseudostyles because the style system is just following the rules that it is 
handed. 

We can cache it in the LinkStateManager (WebShell), or better yet the 
GlobalHistory could make the lookup faster so the cost of asking for a link's 
state would not be so high. To cache this in the Style System seems like a big 
mistake, since the style system would then need to be notified when the state of 
a link changed, and that is what the LinkStateManger is for in the first place. 
Maybe we could cache the state of the link within the duration of one 
enumeration through the style rules, since the link state cannot change during 
that time... I'll see if that is at all feasible.

I'm changing the summary since it is wrong. The real problem is that the cost of 
calling GetLinkState is high. Style is calling it for each rule that relies on 
the state of a link.
Summary: [PERF] GetLinkState is called too often → GetLinkState is expensive and it is called for every :link pseudoclass rule
warren suggested that it might make sense to just skip *all* link lookup during 
initial layout, and then do some kind of second pass to color the links

pierre said that this might be possible by schduling a separate style reflow. 
Does this seem reasonable?

In separate tests that I've run, removing *all* link lookup (e.g., by just 
commenting out the line) speeds up pages by as much as 3x! Granted, those are 
degenerate pages made up of all anchor tags...
If a page has a lot of links, wouldn't the user want to see the links as the 
page loads? *Not* applying the link styles would make the page initially look 
like it was all text, then later, after the page has loaded, the links would be 
styled. This would probably confuse people to no end (it would confuse me). 

Then again, we could assume that all links were in the :link state and style 
them initially like that. Then, during a style reflow, we could apply the actual 
link-state (:linl, :visited and :out-of-date) - that would probably look 
reasonable I think...
Yes, that's what I meant. ;-)
I tried the two-pass link-styling and it works, but I think it is of limited 
value since we already incrementally display content while loading. By doing a 
second-pass link styling we are really just moving the time from document-load 
to just after document load, and the user probably won't notice. I'd prefer to 
just make the link-state determination faster for whenever we do it...

Another idea is to get the href in canonical form once, the first time it is 
needed, and then cache it for subsequent use. We could even cache it as a char* 
so that we don't need to do any nasty conversion before passing it to the link 
handler... (This could happen on first access or in the content-sink when the 
attribute is set) If the canonical form of the href doesn't have to be computed, 
and we don't have to malloc memory or convert data, the all we really have left 
is the lookup, which we can *maybe* optimize if necessary. Should be pretty 
straight-forward to implement too! Joy :)
I added a dependency on 10373 even though it is not really dependent - I just 
wanted to track these two together since the proposed solutions to 10373 may 
help this bug as well.
Depends on: 10373
Ok, so the patch I just attached uses nsIDOMHTMLAnchorElement::GetHref() to get 
the canonical (yay!) URL of the link. I'm going tinker with 
nsHTMLAnchorElement::GetHref() so that it will cache the value after computing 
it once. (Trying to compute the value when it's set ends up being a bit tricky 
because of all the delegation that happens with HTML content elements.)

It may make sense to make a s3kret nsIAnchorContent (a la nsIContent) interface 
that has a GetHref(const char**) method on it: that would be the most efficient 
way to wade through this mess (avoids inflating/deflating the URL repeatedly).
Ok, I've verified that this actually improves life: speeds up style resolution
on the "40 URLs" by anywhere from 5 to 20%. I'll check it in tomorrow.
Chris, you de man!
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
travis@netscape.com: not sure how to verify this bug is fixed. Can you please 
review and if you agree with fix, mark it verified? Thanks
Whiteboard: 8/11: Requested verification by reporter
Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
code level fix with no response from reporter; rubber-stamp-verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.