Make parsing SVG transform lists more efficient by smfr · Pull Request #42822 · WebKit/WebKit

smfr · 2025-03-21T17:34:05Z

`bef74b7`

Make parsing SVG transform lists more efficient
https://bugs.webkit.org/show_bug.cgi?id=290192
rdar://147591008

Reviewed by Said Abou-Hallawa.

Content that animates SVG transforms will often keep the same list of transform operations, but just
change the values. Currently, `SVGTransformList::parse()` implements this by clearing the list and
re-build it, but this involves a lot of object churn: each SVGTransform is heap-allocated, and itself
has a heap-allocated SVGMatrix. In addition, `detach()` and `attach()` have overhead.

Make this more efficient by re-using the existing SVGTransforms when possible. Do this by moving the
list management into `SVGTransformList::parseGeneric()`. Callers of `parse()` expect the list to be preserved
between calls, so the `ListReplacement` argument controls that behavior. Pass the existing object to
`SVGTransformable::parseTransform` if the type matches; we compare that with the return value to know
if the item was re-used. If not, we go back to appending.

Well tested by existing SVG tests.

* Source/WebCore/svg/SVGTransformList.cpp:
(WebCore::SVGTransformList::parseGeneric):
(WebCore::SVGTransformList::parse):
* Source/WebCore/svg/SVGTransformList.h:
* Source/WebCore/svg/SVGTransformable.cpp:
(WebCore::parseTransformGeneric):
(WebCore::SVGTransformable::parseTransform):
(WebCore::parseTransformValueGeneric): Deleted.
(WebCore::SVGTransformable::parseTransformValue): Deleted.
* Source/WebCore/svg/SVGTransformable.h:

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

09770fb

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-03-21T17:34:15Z

EWS run on previous version of this PR (hash 3d5370c)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🛠 wpe-cairo
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	❌ 🛠 gtk
	✅ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	~~🛠 playstation~~
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

shallawa · 2025-03-21T18:04:39Z

I think ListReplacement makes things a little bit more complicated. Why do not we make this function work in the replace mode as long as the type of the existingTransform matches the type of parsedTransform? And once the type differ, just call resize(itemIndex); and this will delete the remaining of all the existingTransforms

I don't think that's possible, because some callers call it multiple times and expect the transforms to accumulate.

shallawa · 2025-03-21T18:08:58Z

It is not clear why we clearItems() when an error happens but only when ListReplacement::Replace. I understand you want to keep the existing behavior. But I think moving clearItems() back to SVGTransformList::parse(StringView value) make reading this function easier.

shallawa · 2025-03-21T18:10:03Z

Can be a for loop:

for (size_t itemIndex = 0; buffer.hasCharactersRemaining(); ++itemIndex)

shallawa · 2025-03-21T18:37:44Z

If calling clearItems(); was moved back to SVGTransformList::parse(StringView value), there is no need for put the parsing code in a lambda.

shallawa · 2025-03-21T18:39:26Z

I think this equivalent to

if (existingTransform && parsedTransform.get() != existingTransform.get()) {

shallawa · 2025-03-21T18:41:34Z

Instead of adding ASSERT to each switch case, we can add one ASSERT at the beginning

ASSERT_IMPLIES(reusableValue, type == reusableValue->value().type());

shallawa · 2025-03-21T18:43:38Z

Should we use the same name the caller passes above? reusableValue -> existingTransform?

Or at least do not use Value in the name to not confuse it with SVGTransformValue. reusableValue -> reusableTransform?

webkit-early-warning-system · 2025-03-21T19:14:46Z

EWS run on previous version of this PR (hash 96b298f)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🛠 gtk
	✅ 🛠 vision	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-03-21T21:44:19Z

EWS run on previous version of this PR (hash 74da465)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	❌ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

shallawa · 2025-03-22T12:33:04Z

Good that you caught this case. But itemIndex < m_items.size() implies currentListReplacement == ListReplacement::Replace i.e. it is not possible that the first clause is true and the second one is false.

webkit-early-warning-system · 2025-03-22T19:24:59Z

EWS run on current version of this PR (hash 09770fb)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=290192 rdar://147591008 Reviewed by Said Abou-Hallawa. Content that animates SVG transforms will often keep the same list of transform operations, but just change the values. Currently, `SVGTransformList::parse()` implements this by clearing the list and re-build it, but this involves a lot of object churn: each SVGTransform is heap-allocated, and itself has a heap-allocated SVGMatrix. In addition, `detach()` and `attach()` have overhead. Make this more efficient by re-using the existing SVGTransforms when possible. Do this by moving the list management into `SVGTransformList::parseGeneric()`. Callers of `parse()` expect the list to be preserved between calls, so the `ListReplacement` argument controls that behavior. Pass the existing object to `SVGTransformable::parseTransform` if the type matches; we compare that with the return value to know if the item was re-used. If not, we go back to appending. Well tested by existing SVG tests. * Source/WebCore/svg/SVGTransformList.cpp: (WebCore::SVGTransformList::parseGeneric): (WebCore::SVGTransformList::parse): * Source/WebCore/svg/SVGTransformList.h: * Source/WebCore/svg/SVGTransformable.cpp: (WebCore::parseTransformGeneric): (WebCore::SVGTransformable::parseTransform): (WebCore::parseTransformValueGeneric): Deleted. (WebCore::SVGTransformable::parseTransformValue): Deleted. * Source/WebCore/svg/SVGTransformable.h: Canonical link: https://commits.webkit.org/292545@main

webkit-commit-queue · 2025-03-22T21:20:21Z

Committed 292545@main (bef74b7): https://commits.webkit.org/292545@main

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

smfr self-assigned this Mar 21, 2025

smfr added the SVG For bugs in the SVG implementation. label Mar 21, 2025

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 21, 2025

shallawa approved these changes Mar 21, 2025

View reviewed changes

smfr removed the merging-blocked Applied to prevent a change from being merged label Mar 21, 2025

smfr force-pushed the eng/Make-parsing-SVG-transform-lists-more-efficient branch from 3d5370c to 96b298f Compare March 21, 2025 19:14

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 21, 2025

smfr removed the merging-blocked Applied to prevent a change from being merged label Mar 21, 2025

smfr force-pushed the eng/Make-parsing-SVG-transform-lists-more-efficient branch from 96b298f to 74da465 Compare March 21, 2025 21:44

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 21, 2025

shallawa reviewed Mar 22, 2025

View reviewed changes

smfr removed the merging-blocked Applied to prevent a change from being merged label Mar 22, 2025

smfr force-pushed the eng/Make-parsing-SVG-transform-lists-more-efficient branch from 74da465 to 09770fb Compare March 22, 2025 19:24

smfr added the merge-queue Applied to send a pull request to merge-queue label Mar 22, 2025

webkit-commit-queue force-pushed the eng/Make-parsing-SVG-transform-lists-more-efficient branch from 09770fb to bef74b7 Compare March 22, 2025 21:20

webkit-commit-queue merged commit bef74b7 into WebKit:main Mar 22, 2025

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 22, 2025

smfr deleted the eng/Make-parsing-SVG-transform-lists-more-efficient branch June 16, 2025 15:24

Conversation

smfr commented Mar 21, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Mar 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smfr commented Mar 21, 2025 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Mar 21, 2025 •

edited

Loading

webkit-early-warning-system commented Mar 21, 2025 •

edited

Loading

webkit-early-warning-system commented Mar 21, 2025 •

edited

Loading

webkit-early-warning-system commented Mar 22, 2025 •

edited

Loading