Closed Bug 1552719 Opened 5 months ago Closed 5 months ago

nsBulletFrame::Ordinal is slow

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- verified
firefox69 --- verified

People

(Reporter: heycam, Assigned: emilio)

References

(Regression)

Details

(Keywords: perf, regression, Whiteboard: [qf])

Attachments

(4 files)

Since switching to the new bullet implementation, I see a lot of time spent under nsBulletFrame::Ordinal.

For example:

Do you know if we spend more time than before on the frame sorting code? I'm surprised that it being such a massive part of page load time according to your profiles it wasn't detected as a perf regression by Talos.

Flags: needinfo?(cam)

We should probably just stash the counter value on the bullet frame here, and make nsBulletFrame::Ordinal() O(1) again.

I think that if the marker uses content then that should already be handled by the SetText() call. Wdyt mats? I'm happy to write a patch for that.

Flags: needinfo?(mats)

Sounds good to me. Could you also remove the aDebugFromA11y param
we added in bug 1539656 too then? (essentially just revert that
patch modulo the #ifdef ACCESSIBILITY it added)

Flags: needinfo?(mats)

Alright, will do :)

Flags: needinfo?(emilio)

[Triaging as a p3 regression from bug 205202, w/ status flags set accordingly. If any of my adjustments are wrong, apologies & please correct.]

Keywords: regression
Priority: -- → P3
Regressed by: 205202
Whiteboard: [qf]

(er, I'll bet this is more specifically a regression from bug 288704 " Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation")

Regressed by: 288704
No longer regressed by: 205202

Err, the orange in that try run is because I forgot to update an assertion... Other than that it looks green, except for the nested pseudo-element case that my own test caught (dammit ;)).

I think there's some counters code assuming that nested pseudos don't exist. I haven't dug yet.

Assignee: nobody → emilio

I thought I was going to need it but turns out I don't. Still this is worth it I
think.

I'm going to need it to fix the counters code in presence of nested
pseudo-elements.

Depends on D31987

When you have a ::after::marker, and you compare one against the other we ended
up with the wrong result because of the pseudotype stuff.

I think this is cleaner now that DoCompareTreePosition handles pseudos properly
(which is really the thing this was working around).

Depends on D31988

I did this instead of just (ab)using the fact that every list item has at least
one counter-increment node because:

  • I don't have the bullet frame around by the time we initially compute the
    counter increment, which means that I'd need to grow nsBlockFrame / add a
    frame property for the list item ordinal, which I think would be unfortunate.

  • It feels more consistent with the way regular CSS counters work and with the
    way we want ::marker to eventually work.

Depends on D31989

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d008903b2ace
Do some cleanup in the counter initializer code. r=mats
https://hg.mozilla.org/integration/autoland/rev/e7dcf2a302c5
Make nsLayoutUtils::DoCompareTreePosition handle pseudos more diligently. r=mats
https://hg.mozilla.org/integration/autoland/rev/209e8a6ae29d
Make nsGenConList::NodeAfter handle correctly nested pseudo-elements. r=mats
https://hg.mozilla.org/integration/autoland/rev/1a44a048e2f0
Make nsBulletFrame::Ordinal() O(1) again. r=mats
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/f023603f7e72
Bump an assertion count in the XBL + lists test, since we run the code that asserts more often now. r=bustage
Blocks: 1544951
Flags: needinfo?(emilio)
No longer blocks: 1544951
Flags: needinfo?(cam)

Comment on attachment 9066373 [details]
Bug 1552719 - Make nsBulletFrame::Ordinal() O(1) again. r=mats

Beta/Release Uplift Approval Request

  • User impact if declined: Perf regression.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See bug 1544951 comment 0.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively straight-forward patches, plus that part of the code is pretty well covered by automated tests.
  • String changes made/needed: none
Attachment #9066373 - Flags: approval-mozilla-beta?
Attachment #9066371 - Flags: approval-mozilla-beta?
Attachment #9066372 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hi Guys, I managed to reproduce this issue in Firefox 68.0b3 and there's a definite improvement in Firefox 69 but the blank page is still displayed for a bit while on Chrome its almost non existent. Can I mark this as Verified is the half a second of a blank page acceptable or it should be similar to Chrome ?

Flags: needinfo?(emilio)

You should see whether Firefox 69 performs ~similar to Firefox 67. Firefox 68 should be slower than that because of the issue this patches fix.

Flags: needinfo?(emilio)

Unfortunately no, Firefox 67 loads the page instantly, compared to that there's a visible difference, but compared to 68 its slightly improved.

Flags: needinfo?(emilio)

So you're saying that 67 is much faster than 68, and still a bit faster than 69?

Flags: needinfo?(emilio) → needinfo?(rares.doghi)

Please disregard my previous comment, I have downloaded the latest Nightly from https://tools.taskcluster.net/index/gecko.v2.mozilla-central.latest.firefox/win64-opt and the page loads Instantly, I will mark this issue accordingly.

Flags: needinfo?(rares.doghi)

Nice :)

Comment on attachment 9066371 [details]
Bug 1552719 - Make nsLayoutUtils::DoCompareTreePosition handle pseudos more diligently. r=mats,mattwoodrow

perf regression in 68, approved for 68.0b4

Attachment #9066371 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9066372 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9066373 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hi, This issue is verified as fixed in our latest Beta 68.0b7. I will mark this issue accordingly.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.