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.

Sign up for GitHub

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

Jump to bottom

DocumentSharedObjectPool prevents sharing of ElementData in case of hash collision #11681

Merged
merged 1 commit into from
Mar 20, 2023
Merged

DocumentSharedObjectPool prevents sharing of ElementData in case of hash collision #11681

merged 1 commit into from
Mar 20, 2023

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Mar 18, 2023 •
edited by webkit-early-warning-system

89317dd

DocumentSharedObjectPool prevents sharing of ElementData in case of hash collision
https://bugs.webkit.org/show_bug.cgi?id=254114

Reviewed by Darin Adler.

Use a HashSet of RefPtr<ShareableElementData> for the cache, with proper traits
and hashing so that it does what we expect. This means we can let the HashSet
implementation deal with hash collision and we can still share ElementData in
the event of such collision.

The previous code was using a HashMap whose key was the hash and thus couldn't
deal with hash collision and would just not share in this case.

This is performance neutral on Speedometer on both iPhone and Mac.

* Source/WebCore/dom/DocumentSharedObjectPool.cpp:
(WebCore::DocumentSharedObjectPool::ShareableElementDataHash::hash):
(WebCore::DocumentSharedObjectPool::ShareableElementDataHash::equal):
(WebCore::AttributeSpanTranslator::hash):
(WebCore::AttributeSpanTranslator::equal):
(WebCore::AttributeSpanTranslator::translate):
(WebCore::DocumentSharedObjectPool::cachedShareableElementDataWithAttributes):
(WebCore::hasSameAttributes): Deleted.
* Source/WebCore/dom/DocumentSharedObjectPool.h:

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

4bd9361

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 ⏳ 🛠 gtk
⏳ 🧪 webkitpy ⏳ 🧪 api-ios ⏳ 🧪 mac-wk1 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🛠 jsc-armv7
⏳ 🧪 services ⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🧪 jsc-armv7-tests
⏳ 🛠 watch-sim ⏳ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ⏳ 🧪 jsc-mips-tests

@cdumez cdumez requested a review from rniwa as a code owner March 18, 2023 16:39
@cdumez cdumez self-assigned this Mar 18, 2023
@cdumez cdumez added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Mar 18, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 5dad145)

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 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 18, 2023
darinadler
darinadler approved these changes Mar 19, 2023
View reviewed changes
Source/WebCore/dom/DocumentSharedObjectPool.cpp Outdated
return equalAttributes(reinterpret_cast<const uint8_t*>(attributes.data()), reinterpret_cast<const uint8_t*>(elementData.m_attributeArray), attributes.size() * sizeof(Attribute));
}
struct DocumentSharedObjectPool::ShareableElementDataHash {
static unsigned hash(const RefPtr<ShareableElementData>& o)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe data instead of o.

Source/WebCore/dom/DocumentSharedObjectPool.cpp Outdated
struct DocumentSharedObjectPool::ShareableElementDataHash {
static unsigned hash(const RefPtr<ShareableElementData>& o)
{
ASSERT_WITH_MESSAGE(o.get(), "attempt to use null RefPtr in HashTable");
Copy link
Member

Choose a reason for hiding this comment

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

Can you omit the get() here? Also, I am pretty sure we support Ref, not just RefPtr, as a hash table key. Seems like that’s called for here.

@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Mar 19, 2023
@cdumez cdumez force-pushed the 254114_cachedShareableElementDataWithAttributes branch from 5dad145 to 725562a Compare March 19, 2023 22:17
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 725562a)

Misc iOS, tvOS & watchOS macOS Linux Windows
⏳ 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 wincairo
loading 🧪 bindings ⏳ 🛠 ios-sim ⏳ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
⏳ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac ⏳ 🛠 gtk
⏳ 🧪 api-ios ⏳ 🧪 mac-wk1 ⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🛠 jsc-armv7
⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🧪 jsc-armv7-tests
⏳ 🛠 watch-sim loading 🛠 jsc-mips
loading 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 725562a)

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 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

darinadler
darinadler reviewed Mar 20, 2023
View reviewed changes
Source/WebCore/dom/DocumentSharedObjectPool.cpp Outdated

// FIXME: This prevents sharing when there's a hash collision.
if (cachedData && !hasSameAttributes(attributes, *cachedData))
return m_shareableElementDataCache.ensure<AttributeSpanTranslator>(attributes, [attributes] {
Copy link
Member

Choose a reason for hiding this comment

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

Given the translator already lazily creates the ShareableElementData, couldn’t we just use add instead of ensure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe you're right. I'll update the patch.

@cdumez cdumez force-pushed the 254114_cachedShareableElementDataWithAttributes branch from 725562a to d0baa49 Compare March 20, 2023 15:03
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash d0baa49)

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

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Mar 20, 2023
@webkit-commit-queue webkit-commit-queue force-pushed the 254114_cachedShareableElementDataWithAttributes branch from d0baa49 to 4bd9361 Compare March 20, 2023 19:45
@cdumez cdumez added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merge-queue Applied to send a pull request to merge-queue labels Mar 20, 2023
@cdumez
DocumentSharedObjectPool prevents sharing of ElementData in case of h…
89317dd 
…ash collision

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

Reviewed by Darin Adler.

Use a HashSet of RefPtr<ShareableElementData> for the cache, with proper traits
and hashing so that it does what we expect. This means we can let the HashSet
implementation deal with hash collision and we can still share ElementData in
the event of such collision.

The previous code was using a HashMap whose key was the hash and thus couldn't
deal with hash collision and would just not share in this case.

This is performance neutral on Speedometer on both iPhone and Mac.

* Source/WebCore/dom/DocumentSharedObjectPool.cpp:
(WebCore::DocumentSharedObjectPool::ShareableElementDataHash::hash):
(WebCore::DocumentSharedObjectPool::ShareableElementDataHash::equal):
(WebCore::AttributeSpanTranslator::hash):
(WebCore::AttributeSpanTranslator::equal):
(WebCore::AttributeSpanTranslator::translate):
(WebCore::DocumentSharedObjectPool::cachedShareableElementDataWithAttributes):
(WebCore::hasSameAttributes): Deleted.
* Source/WebCore/dom/DocumentSharedObjectPool.h:

Canonical link: https://commits.webkit.org/261893@main
@webkit-commit-queue webkit-commit-queue force-pushed the 254114_cachedShareableElementDataWithAttributes branch from 4bd9361 to 89317dd Compare March 20, 2023 23:26
@webkit-commit-queue webkit-commit-queue merged commit 89317dd into WebKit:main Mar 20, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 261893@main (89317dd): https://commits.webkit.org/261893@main

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

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darinadler darinadler darinadler approved these changes

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@anttijk anttijk Awaiting requested review from anttijk

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

Assignees

@cdumez cdumez

Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
Milestone
No milestone
5 participants
@cdumez @webkit-early-warning-system @webkit-commit-queue @darinadler @webkit-ews-buildbot