Bug 239576 – Adopt RobinHoodHashMap / RobinHoodHashSet more broadly in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Adopt RobinHoodHashMap / RobinHoodHashSet more broadly in WebCore to avoid wasting memory in hash tables. RobinHoodHashMap / RobinHoodHashSet have more restrictions on what key types they work with and may result in slower performance but they have a much higher load factor that the regular HashMap / HashSet, thus reducing memory usage. This patch adopts RobinHoodHashMap / RobinHoodHashSet on non performance sensitive maps / sets in WebCore that have compatible keys (String / AtomString / URL because they cache their hash).

[Chris Dumez]

Created attachment 458021 [details]
Patch

[Yusuke Suzuki]

Interesting. Some of audio tests are failing because of (probably) hashtable ordering. Either or both of,

1. Our implementation is relying on HashTable ordering, which is undefined.
2. The test result is relying on HashTable ordering, which is undefined.

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #2)
> Interesting. Some of audio tests are failing because of (probably) hashtable
> ordering. Either or both of,
> 
> 1. Our implementation is relying on HashTable ordering, which is undefined.
> 2. The test result is relying on HashTable ordering, which is undefined.

I'll fix it tomorrow, shouldn't be difficult. I am waiting on perf A/B results anyway.

[Yusuke Suzuki]

(In reply to Chris Dumez from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > Interesting. Some of audio tests are failing because of (probably) hashtable
> > ordering. Either or both of,
> > 
> > 1. Our implementation is relying on HashTable ordering, which is undefined.
> > 2. The test result is relying on HashTable ordering, which is undefined.
> 
> I'll fix it tomorrow, shouldn't be difficult. I am waiting on perf A/B
> results anyway.

Nice

[Chris Dumez]

Created attachment 458037 [details]
Patch

[Chris Dumez]

Created attachment 458066 [details]
Patch

[Chris Dumez]

Created attachment 458067 [details]
Patch

[Chris Dumez]

Created attachment 458081 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 458081 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 458081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=458081&action=review

> Source/WebCore/ChangeLog:15
> +        This is perf-neutral on all our benchmarks according to A/B testing.

Does memory use go down?

[EWS]

Committed r293195 (249870@main): <https://commits.webkit.org/249870@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458081 [details].

[Radar WebKit Bug Importer]

<rdar://problem/92128851>

[Chris Dumez]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 458081 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458081&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        This is perf-neutral on all our benchmarks according to A/B testing.
> 
> Does memory use go down?

I didn't check PLUM on iOS (I can look at the bot now that it landed). Sadly it didn't move the needle on Membuster on macOS according to A/B testing, I was hoping it would.