1732624 - Convert XPConnect hash tables to use mozilla::HashMap rather than PLDHashTable

Status: RESOLVED FIXED

(Core :: XPConnect, task, P3)

Description

[Jon Coppeard (:jonco)]

mozilla::HashMap and HashSet are based on templates and offer better performance compared to PLDHashTable because they offer more opportunity for inlining.

This change also offers the possibility of better integration with the JS GC's WeakCache API where hash tables are swept automatically rather than having to do this manually in a callback.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 1: Convert Native2WrappedNativeMap to use mozilla::HashMap r?mccr8

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 2: Convert IID2NativeInterfaceMap to use mozilla::HashMap r?mccr8

Depends on D126639

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 3: Convert ClassInfo2NativeSetMap to use mozilla::HashMap r?mccr8

Depends on D126640

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 4: Convert ClassInfo2WrappedNativeProtoMap to use mozilla::HashMap r?mccr8

Depends on D126641

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 5: Convert NativeSetMap to use mozilla::HashSet r?mccr8

Confusingly it seems the matching function used with mozilla::HashSet receives
its arguments the other way round to the one for PLDHashTable. At least with
HashSet it is typed.

I didn't change the class's name although arguably it should be called NativeSetSet.

Depends on D126642

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 6: Convert XPCWrappedNativeProtoMap to use mozilla::HashMap r?mccr8

Depends on D126643

Comment 7

[Chris Peterson [:cpeterson]]

Nighty 94's code freeze is only three days from now. We probably shouldn't land such a big change this late in the release cycle.

Comment 8

[Chris Peterson [:cpeterson]]

needinfo'ing Nika because she may have opinions about PLDHashTable, mozilla::HashMap, and nsTHashtable.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #0)

mozilla::HashMap and HashSet are based on templates and offer better performance compared to PLDHashTable because they offer more opportunity for inlining.

This change also offers the possibility of better integration with the JS GC's WeakCache API where hash tables are swept automatically rather than having to do this manually in a callback.

What is the motivation for these patches? Are you seeing instances where the performance of these hash tables is an issue? There's a tradeoff between binary size and performance between these two implementations (although there are likely not a ton of call sites for operations on these hash tables so hopefully it won't be an issue.)

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
For the tables that are swept as part of GC, if we can make them be GCHashMap then we can use WeakCache<GCHashMap> and have them swept in parallel with other things need updating. Probably not a huge win but nice to exploit if we can. But also it seems a nice tidyup to use the mozilla::HashMap API and remove the need to cast when iterating the maps etc.

The binary size is a good point, and something I hadn't considered. I measured this for clean release builds on linux and found that these changes decreased the size of libxul.so by 38KB, although I don't understand how.

Pre:  -rwxr-x--- 1 jon jon 169309688 Sep 28 09:54 opt-build/dist/firefox/libxul.so
Post: -rwxr-x--- 1 jon jon 169271176 Sep 28 10:06 opt-build/dist/firefox/libxul.so

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1732624 - Part 7: Remove XPCMaps.h include from xpcprivate.h r?mccr8

The latter header gets included in a whole bunch of places outside XPConnect,
and it's not necessary to include XPCMaps.h in it.

Depends on D126644

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc520dac1739
Part 1: Convert Native2WrappedNativeMap to use mozilla::HashMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/d21d9cac6ae6
Part 2: Convert IID2NativeInterfaceMap to use mozilla::HashMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/58d727381614
Part 3: Convert ClassInfo2NativeSetMap to use mozilla::HashMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/f8d7ce4f7508
Part 4: Convert ClassInfo2WrappedNativeProtoMap to use mozilla::HashMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/1dcdb11508ed
Part 5: Convert NativeSetMap to use mozilla::HashSet r=mccr8
https://hg.mozilla.org/integration/autoland/rev/99dc0e64be7f
Part 6: Convert XPCWrappedNativeProtoMap to use mozilla::HashMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/560ee149e587
Part 7: Remove XPCMaps.h include from xpcprivate.h r=mccr8

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/dc520dac1739
https://hg.mozilla.org/mozilla-central/rev/d21d9cac6ae6
https://hg.mozilla.org/mozilla-central/rev/58d727381614
https://hg.mozilla.org/mozilla-central/rev/f8d7ce4f7508
https://hg.mozilla.org/mozilla-central/rev/1dcdb11508ed
https://hg.mozilla.org/mozilla-central/rev/99dc0e64be7f
https://hg.mozilla.org/mozilla-central/rev/560ee149e587