Closed
Bug 29611
Opened 25 years ago
Closed 24 years ago
GetLinkState is expensive and it is called for every :link pseudoclass rule
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
VERIFIED
FIXED
M17
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?
Comment 1•25 years ago
|
||
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.
Assignee | ||
Comment 3•25 years ago
|
||
Assigning to Waqar: 1/3 of Pierre's NEW bugs to help reduce his doomage factor
Assignee: pierre → waqar
Comment 4•25 years ago
|
||
Reassigned back to me these bugs that shouldn't have left my list.
Assignee: waqar → pierre
Assignee | ||
Comment 5•25 years ago
|
||
Taking performance related style bugs that are still NEW
Assignee: pierre → attinasi
Assignee | ||
Comment 6•25 years ago
|
||
This is a performance problem. I'll look into it after beta2
Assignee | ||
Comment 7•24 years ago
|
||
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
Comment 8•24 years ago
|
||
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...
Assignee | ||
Comment 9•24 years ago
|
||
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...
Comment 10•24 years ago
|
||
Yes, that's what I meant. ;-)
Assignee | ||
Comment 11•24 years ago
|
||
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 :)
Assignee | ||
Comment 12•24 years ago
|
||
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
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
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).
Comment 15•24 years ago
|
||
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.
Assignee | ||
Comment 16•24 years ago
|
||
Chris, you de man!
Comment 17•24 years ago
|
||
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 18•24 years ago
|
||
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
Comment 19•24 years ago
|
||
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
Comment 20•23 years ago
|
||
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.
Description
•