Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not synchronously measure SVG text every time it changes #21278

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Dec 4, 2023

97f84b2

Do not synchronously measure SVG text every time it changes
https://bugs.webkit.org/show_bug.cgi?id=264669
rdar://problem/118451741

Reviewed by Simon Fraser.

This is a optimization done in the Blink fork https://chromium.googlesource.com/chromium/src.git/+/34c351416a102e4ee510badb86fbc4f57604ccd0 and found by Ahmad Saleem.

If SVG text is set multiple times in a row, we would
measure text synchronously for every set, even though just the last
measurement would count. This patch marks it as "need layout", so
we measure text in the next layout.

* Source/WebCore/rendering/svg/RenderSVGText.cpp:
(WebCore::RenderSVGText::subtreeTextDidChange):

Canonical link: https://commits.webkit.org/271678@main

883a96f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@vitorroriz vitorroriz self-assigned this Dec 4, 2023
@vitorroriz vitorroriz added the SVG For bugs in the SVG implementation. label Dec 4, 2023
@Ahmad-S792
Copy link
Contributor

Ahmad-S792 commented Dec 4, 2023

@vitorroriz - does it show any performance improvement in WebKit through A/B Test or on any specific benchmark?

@vitorroriz
Copy link
Contributor Author

@Ahmad-S792

@vitorroriz - does it show any performance improvement in WebKit through A/B Test or on any specific benchmark?

I don't know yet. I've tried running benchmarks for it before but I was having some build issues. I'm trying again now.

@vitorroriz
Copy link
Contributor Author

A/B tests show no progression/regression for this patch. But since it is a bit specific (multiple consecutive updates of SVG text), it might be our benchmarks don't cover that and that's still a valid patch. Maybe @shallawa can have a look at it.

@vitorroriz vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Dec 7, 2023
https://bugs.webkit.org/show_bug.cgi?id=264669
rdar://problem/118451741

Reviewed by Simon Fraser.

This is a optimization done in the Blink fork https://chromium.googlesource.com/chromium/src.git/+/34c351416a102e4ee510badb86fbc4f57604ccd0 and found by Ahmad Saleem.

If SVG text is set multiple times in a row, we would
measure text synchronously for every set, even though just the last
measurement would count. This patch marks it as "need layout", so
we measure text in the next layout.

* Source/WebCore/rendering/svg/RenderSVGText.cpp:
(WebCore::RenderSVGText::subtreeTextDidChange):

Canonical link: https://commits.webkit.org/271678@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Do-not-synchronously-measure-SVG-text-every-time-it-changes branch from 883a96f to 97f84b2 Compare December 7, 2023 17:40
@webkit-commit-queue
Copy link
Collaborator

Committed 271678@main (97f84b2): https://commits.webkit.org/271678@main

Reviewed commits have been landed. Closing PR #21278 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 97f84b2 into WebKit:main Dec 7, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 7, 2023
Comment on lines -291 to -294
for (RenderObject* descendant = text; descendant; descendant = descendant->nextInPreOrder(text)) {
if (is<RenderSVGInlineText>(*descendant))
m_layoutAttributesBuilder.buildLayoutAttributesForTextRenderer(downcast<RenderSVGInlineText>(*descendant));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is so special about subtreeTextDidChange()? The same function buildLayoutAttributesForTextRenderer() is called from subtreeChildWasAdded() and subtreeChildWasRemoved() in the same file. So why do not we apply the same fix in these two functions as well?

Also are we sure this will not affect JS querying the SVG text in the same update-the-rendering this function is called?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SVG For bugs in the SVG implementation.
Projects
None yet
6 participants