661768 - Defer history lookup on anchor creation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Randell Jesup [:jesup] (needinfo me)]

When doing BindToTree() for an anchor element in the Dromaeo tests, it calls UpdateState() which calls LinkState(), which looks up the URI in a history hash.  This blocks possible performance improvements which depend on deferring URI normalization (see  bug 268200).

Since displaying a link requires coloring it properly, it will probably need to be normalized when displayed.  If having been visited affects layout, then it would need to be normalized on reflow/layout.  If JS code checks the link status, that would require normalization.  There are probably other cases, and I'm not sure it can be deferred enough to make bug 268200's plan to defer normalization effective (at least in non-benchmark cases).

(filed at bz's request)

Comment 1

[Boris Zbarsky [:bzbarsky]]

General plan is to try constructing the URI as we do now but not starting the history lookup unless we're actually resolving style, so that unrelated state updates don't trigger the history lookup.

Comment 2

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

(Assuming you didn't really want to unassign)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note from bug 268200 that it's also doing ResetLinkState() on UnBindFromTree, which causes a history operation.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I believe the combination of bug 1355540 and bug 1378218 fixed this. Does that look about right?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Boris Zbarsky [:bzbarsky] from comment #4)

I believe the combination of bug 1355540 and bug 1378218 fixed this. Does that look about right?

Not sure... that cut the hashtable overhead (using segmentedvector/etc) and removed some other overhead, but I don't think that addresses comment 0 or comment 1. A profile might help.

The idea here was to unblock some of the ideas like https://bugzilla.mozilla.org/show_bug.cgi?id=268200#c13

Comment 6

[Boris Zbarsky [:bzbarsky]]

Looking at the code, the key is that HTMLAnchorElement::BindToTree now calls RegisterPendingLinkUpdate which:

1. Calls SetHasPendingLinkUpdate(), which prevents any history lookup stuff happening until ClearHasPendingLinkUpdate. See the HasPendingLinkUpdate() check in Link::LinkState(), in particular.
2. Adds the link to a list of links to update, which does not involve getting its URI or doing history lookups.
3. Schedules an idle-priority runnable that for each link calls ClearHasPendingLinkUpdate and then if the link is still in the doc calls UpdateLinkState, which is what triggers the history lookup.

So the practical effect is that we defer the history lookup to an idle runnable, which seems like it addresses what comment 0 and comment 1 were about, except more so: even the URI construction is deferred. That's helped by the fact that the "is a link" check now just checks for nonempty href rather than checking that the href could in fact be turned into a valid nsIURI.

Comment 7

[Denis Palmeiro [:denispal]]

Given that this was first brought up 9 years ago, maybe we need some new data to support whether this is still useful? Also, should this be marked as a defect? It sounds like an enhancement instead. Marking it as qf:p3 unless we have some recent data to support it's need.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 9

[Kris Maglione [:kmag]]

I believe this is fixed.

Note also that layout shouldn't depend on :visited state anymore. We now only allow :visited rules to change the link color now, and we lie about the color to JS consumers for security reasons.

Comment 10

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

Yes, I think comment 6 agrees that this is fixed.

(In reply to Kris Maglione [:kmag] from comment #9)

Note also that layout shouldn't depend on :visited state anymore. We now only allow :visited rules to change the link color now, and we lie about the color to JS consumers for security reasons.

That was true when this was filed; see bug 147777 comment 249, which predates this bug by about 14 months.

Comment 11

[Kris Maglione [:kmag]]

(In reply to David Baron :dbaron: from comment #10)

Yes, I think comment 6 agrees that this is fixed.

(In reply to Kris Maglione [:kmag] from comment #9)

Note also that layout shouldn't depend on :visited state anymore. We now only allow :visited rules to change the link color now, and we lie about the color to JS consumers for security reasons.

That was true when this was filed; see bug 147777 comment 249, which predates this bug by about 14 months.

I know, I checked. But comment 0 mentioned reflow/layout and no other comment mentioned it, so I thought I should for the sake of completion.