Skip to content

Add data structure for memory efficient lists of std::variants#36900

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
weinig:eng/Add-VariantList
Nov 23, 2024
Merged

Add data structure for memory efficient lists of std::variants#36900
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
weinig:eng/Add-VariantList

Conversation

@weinig
Copy link
Copy Markdown
Contributor

@weinig weinig commented Nov 20, 2024

fdc91dd

Add data structure for memory efficient lists of std::variants
https://bugs.webkit.org/show_bug.cgi?id=283423

Reviewed by Darin Adler.

Adds a new data structure, VariantList<std::variant<Ts...>> which acts
like a Vector<std::variant<Ts...>> except that each element only takes up
its own size (+ tag size). The tradeoff is that the list is not random
access capable, and access to the elements is only available via iteration.
In practice, this tradeoff is a good one for the CSS and Style value types.

Rather than vending std::variants, VariantList vends VariantListProxy
objects which act like std::variants, but don't require the extra costs
of construction.

* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/CMakeLists.txt:
* Source/WTF/wtf/StdLibExtras.h:
* Source/WTF/wtf/VariantList.h: Added.
* Source/WTF/wtf/VariantListOperations.h: Added.
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WTF/VariantList.cpp: Added.

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

8ece8d1

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
❌ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
❌ 🛠 🧪 jsc-arm64 ✅ 🛠 vision 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv ❌ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@weinig weinig self-assigned this Nov 20, 2024
@weinig weinig requested review from cdumez and darinadler November 20, 2024 18:55
@weinig
Copy link
Copy Markdown
Contributor Author

weinig commented Nov 20, 2024

I have built these almost entirely using VectorBuffer and std::spans, but I can't quite figure out how to appease the unsafe buffers warnings in a few places where I use a pointer, not to derereference anything, just to do some math to figure out how much we need to grow.

I figure those same problems will have to be solved when updating Vector, so I could crib off that when the time comes.

@weinig weinig requested a review from smfr November 22, 2024 18:02
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the most part, @cdumez has convinced me to just use uint8_t for generic bytes and not start to use std::byte. Maybe you can convince me otherwise, after all uint8_t and LChar are the same type and one advantage of std::byte is that it’s not the same as any other type!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Other than some extra overloads, what's the argument?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess maybe extra type conversions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would think that would be just a temporary problem while making the move from uint8_t to std::byte for actual opaque byte buffers.

But either way, this is entirely constrained to the data structure, so if we decide we really don't like std::byte, changing it will be very easy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This additional overloading is part of the cost of starting to use std::byte. I suppose byteCast sprinkling is less elegant.

Comment on lines 38 to 39
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider putting these into Forward.h instead?

Comment on lines 39 to 40
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would normally omit the type argument name Variant from these forward declarations, but I can see including it if you think it adds clarity. I would also put forward declarations like these above a type definition like VariantListTag.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An idiom like this seems like it needs the comment that explains what its intentions are.

@weinig weinig force-pushed the eng/Add-VariantList branch from 8b0b757 to 8ece8d1 Compare November 23, 2024 03:26
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 23, 2024
@weinig weinig added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Nov 23, 2024
@weinig
Copy link
Copy Markdown
Contributor Author

weinig commented Nov 23, 2024

Doesn't look like any of those failures are related to this PR.

https://bugs.webkit.org/show_bug.cgi?id=283423

Reviewed by Darin Adler.

Adds a new data structure, VariantList<std::variant<Ts...>> which acts
like a Vector<std::variant<Ts...>> except that each element only takes up
its own size (+ tag size). The tradeoff is that the list is not random
access capable, and access to the elements is only available via iteration.
In practice, this tradeoff is a good one for the CSS and Style value types.

Rather than vending std::variants, VariantList vends VariantListProxy
objects which act like std::variants, but don't require the extra costs
of construction.

* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/CMakeLists.txt:
* Source/WTF/wtf/StdLibExtras.h:
* Source/WTF/wtf/VariantList.h: Added.
* Source/WTF/wtf/VariantListOperations.h: Added.
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WTF/VariantList.cpp: Added.

Canonical link: https://commits.webkit.org/287010@main
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 287010@main (fdc91dd): https://commits.webkit.org/287010@main

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

@webkit-commit-queue webkit-commit-queue merged commit fdc91dd into WebKit:main Nov 23, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants