1688833 - Migrate nsBaseHashtable::LookupForAdd to nsBaseHashtable::WithEntryHandle

Status: RESOLVED FIXED

(Core :: XPCOM, task, P2)

Description

[Jan Varga [:janv]]

After bug 1681469, there will be two APIs for LookupForAdd (WithEntryHandle). The new one is based on nsTHashtable::WithEntryHandle which is based on PLDHashTable::WithEntryHandle. The new API isn't currently public, since there are still some open questions which need to be addressed.

During the merge we should:

• Implement EntryHandle::OrInsertFrom
• Check the semantics of PLDHashTable::EntryHandle::operator bool and nsBaseHashtable::EntryPtr::operator bool (might need to check all the call sites)
• Fix nested LookupForAdd, https://searchfox.org/mozilla-central/rev/358fbca0398ac651f5ea6030be39b1870ec180a5/xpcom/tests/gtest/TestHashtables.cpp#646
• Fix the case when other operations are called, https://searchfox.org/mozilla-central/rev/358fbca0398ac651f5ea6030be39b1870ec180a5/netwerk/protocol/http/nsHttpTransaction.cpp#1134
• Consider different names for some of EntryHandle methods
• Consider other cleanup in the API
• Consider having only EntryHandle::OrRemove, no unsafe EntryHandle::Remove method (https://phabricator.services.mozilla.com/D99428#inline-578905)
• Consider changing the way how we call the function in WithEntryHandle (https://phabricator.services.mozilla.com/D99428#inline-578877)

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Make nsBaseHashtable::WithEntryHandle public and complete its design. r=#xpcom-reviewers

Implements the missing handle functions (OrInsertFrom), and harmonizes functions
to return a reference to the data.

Adds unit tests.

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/workers. r=#dom-workers

Depends on D99780

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/serviceworkers. r=#dom-workers

Depends on D104191

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/quota. r=#dom-storage

Depends on D104192

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/localstorage. r=#dom-storage

Depends on D104193

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in xpcom. r=#xpcom-reviewers

Depends on D104194

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in modules/libpref. r=KrisWright

Depends on D104195

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in netwerk. r=#necko-reviewers

Depends on D104196

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in docshell. r=nika

Depends on D104197

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/ipc. r=nika

Depends on D104200

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in layout. r=emilio

Depends on D104201

Comment 12

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/base. r=smaug

Depends on D104202

Comment 13

[Emilio Cobos Álvarez (:emilio)]

What is the reasoning for this new API? It seems much uglier / harder to read than the current one...

Comment 14

[Simon Giesecke [:sg] [he/him]]

There was an extensive discussion on this in a review on Bug 1681469 around https://phabricator.services.mozilla.com/D99428#inline-577669. I'll try to summarize it: The patch added support for non-default-constructible DataTypes/UserDataTypes (such as NotNull<T>) with nsBaseHashtable. It turned out that this would have required changes to LookupForAdd. A similar API was added to the underlying PLDHashtable with somewhat stricter diagnostic checks that guard against modifications to the hashtable while the result object of LookupForAdd is held, which are generally unsafe. It turned out that there is actually code that performs such modifications, which led to the conclusion that this should be scoped better to reduce opportunities for misuse already at compile time. This is what WithEntryHandle (in the patch originally suggested as WithRawEntry) achieves: Since the scope of the entry handle is now restricted to the execution time of the passed functor, typically a lambda expression, it is less likely that the scope of that handle extends to modifications to the hashtable other than through that handle. (Obviously, it does not prevent this, you could still capture the hashtable itself in the lambda and perform other operations, but it's much more visible in the code structure that there is such a scope). One side aspect that was changed is that it avoids adding a new entry (with a default-constructed value), which would then be removed again using OrRemove (which would have required changes anyway to support non-default-constructible DataTypes). Looking at the overall code changes, it is true that some uses get a bit more clumsy, but other uses get simpler.

If you have suggestions on improving WithEntryHandle (including the name) before it lands as a public API, it's probably best to comment on https://phabricator.services.mozilla.com/D99764.

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in gfx/thebes. r=lsalzman

Depends on D104203

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in gfx. r=jgilbert

Depends on D104215

Comment 17

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/storage. r=#dom-storage

Depends on D104216

Comment 18

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in uriloader. r=emilio

Depends on D104217

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/xre. r=tkikuchi

Depends on D104218

Comment 20

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/components/telemetry. r=tkikuchi

Depends on D104219

Comment 21

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/components/places. r=mak

Depends on D104220

Comment 22

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/components/extensions. r=kmag

Depends on D104221

Comment 23

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/components/antitracking. r=timhuang

Depends on D104222

Comment 24

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/payments. r=edenchuang

Depends on D104223

Comment 25

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/media/ogg. r=bryce

Depends on D104224

Comment 26

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/html. r=mattwoodrow

Depends on D104225

Comment 27

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/events. r=smaug

Depends on D104226

Comment 28

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/console. r=nika

Depends on D104227

Comment 29

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/animation. r=emilio

Depends on D104228

Comment 30

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/broadcastchannel. r=baku

Depends on D104229

Comment 31

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/clients. r=#dom-workers

Depends on D104230

Comment 32

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in dom/commandhandler. r=masayuki

Depends on D104231

Comment 33

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1688833 - Change remaining references to LookupForAdd to WithEntryHandle and remove LookupForAdd. r=#xpcom-reviewers

Depends on D104232

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64bec2eefbf7
Make nsBaseHashtable::WithEntryHandle public and complete its design. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/627abcde8bdb
Migrate LookupForAdd to WithEntryHandle in dom/workers. r=janv
https://hg.mozilla.org/integration/autoland/rev/bd3094b10a97
Migrate LookupForAdd to WithEntryHandle in dom/quota. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/a9ec621a6bae
Migrate LookupForAdd to WithEntryHandle in dom/localstorage. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/09a0a70709f4
Migrate LookupForAdd to WithEntryHandle in xpcom. r=xpcom-reviewers,kmag
https://hg.mozilla.org/integration/autoland/rev/6751728d9484
Migrate LookupForAdd to WithEntryHandle in netwerk. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/a311f1bd8063
Migrate LookupForAdd to WithEntryHandle in docshell. r=nika
https://hg.mozilla.org/integration/autoland/rev/bfb7f36f5df1
Migrate LookupForAdd to WithEntryHandle in dom/ipc. r=nika
https://hg.mozilla.org/integration/autoland/rev/f61b74fa2c36
Migrate LookupForAdd to WithEntryHandle in layout. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1b3b997f2e5f
Migrate LookupForAdd to WithEntryHandle in dom/base. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3b89e5552e65
Migrate LookupForAdd to WithEntryHandle in gfx/thebes. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/da6a3d7b6d58
Migrate LookupForAdd to WithEntryHandle in gfx. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/8a570d5a1e50
Migrate LookupForAdd to WithEntryHandle in dom/storage. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/82ea392ecedc
Migrate LookupForAdd to WithEntryHandle in uriloader. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2a04e0655109
Migrate LookupForAdd to WithEntryHandle in toolkit/xre. r=tkikuchi
https://hg.mozilla.org/integration/autoland/rev/5833200749ed
Migrate LookupForAdd to WithEntryHandle in toolkit/components/telemetry. r=tkikuchi
https://hg.mozilla.org/integration/autoland/rev/9c3e24ff5c40
Migrate LookupForAdd to WithEntryHandle in toolkit/components/places. r=mak
https://hg.mozilla.org/integration/autoland/rev/57f42a7a02cd
Migrate LookupForAdd to WithEntryHandle in toolkit/components/extensions. r=kmag
https://hg.mozilla.org/integration/autoland/rev/dee6acdd73e6
Migrate LookupForAdd to WithEntryHandle in toolkit/components/antitracking. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/d5c40257f4b4
Migrate LookupForAdd to WithEntryHandle in dom/media/ogg. r=bryce
https://hg.mozilla.org/integration/autoland/rev/1b63ce5ee296
Migrate LookupForAdd to WithEntryHandle in dom/html. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e5c31dd3a9ce
Migrate LookupForAdd to WithEntryHandle in dom/events. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1d3b99d20807
Migrate LookupForAdd to WithEntryHandle in dom/console. r=nika
https://hg.mozilla.org/integration/autoland/rev/41a6c5fd2986
Migrate LookupForAdd to WithEntryHandle in dom/animation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/409e2d357905
Migrate LookupForAdd to WithEntryHandle in dom/broadcastchannel. r=baku
https://hg.mozilla.org/integration/autoland/rev/16469cbbbabe
Migrate LookupForAdd to WithEntryHandle in dom/clients. r=janv
https://hg.mozilla.org/integration/autoland/rev/ec471701d5ff
Migrate LookupForAdd to WithEntryHandle in dom/commandhandler. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/f476926ccb11
Migrate LookupForAdd to WithEntryHandle in modules/libpref. r=KrisWright
https://hg.mozilla.org/integration/autoland/rev/ebfb9b899c4d
Migrate LookupForAdd to WithEntryHandle in dom/serviceworkers. r=dom-worker-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/87b3efc6b594
Migrate LookupForAdd to WithEntryHandle in dom/payments. r=edenchuang
https://hg.mozilla.org/integration/autoland/rev/d092f5abddc8
Change remaining references to LookupForAdd to WithEntryHandle and remove LookupForAdd. r=xpcom-reviewers,nika

Comment 35

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/64bec2eefbf7
https://hg.mozilla.org/mozilla-central/rev/627abcde8bdb
https://hg.mozilla.org/mozilla-central/rev/bd3094b10a97
https://hg.mozilla.org/mozilla-central/rev/a9ec621a6bae
https://hg.mozilla.org/mozilla-central/rev/09a0a70709f4
https://hg.mozilla.org/mozilla-central/rev/6751728d9484
https://hg.mozilla.org/mozilla-central/rev/a311f1bd8063
https://hg.mozilla.org/mozilla-central/rev/bfb7f36f5df1
https://hg.mozilla.org/mozilla-central/rev/f61b74fa2c36
https://hg.mozilla.org/mozilla-central/rev/1b3b997f2e5f
https://hg.mozilla.org/mozilla-central/rev/3b89e5552e65
https://hg.mozilla.org/mozilla-central/rev/da6a3d7b6d58
https://hg.mozilla.org/mozilla-central/rev/8a570d5a1e50
https://hg.mozilla.org/mozilla-central/rev/82ea392ecedc
https://hg.mozilla.org/mozilla-central/rev/2a04e0655109
https://hg.mozilla.org/mozilla-central/rev/5833200749ed
https://hg.mozilla.org/mozilla-central/rev/9c3e24ff5c40
https://hg.mozilla.org/mozilla-central/rev/57f42a7a02cd
https://hg.mozilla.org/mozilla-central/rev/dee6acdd73e6
https://hg.mozilla.org/mozilla-central/rev/d5c40257f4b4
https://hg.mozilla.org/mozilla-central/rev/1b63ce5ee296
https://hg.mozilla.org/mozilla-central/rev/e5c31dd3a9ce
https://hg.mozilla.org/mozilla-central/rev/1d3b99d20807
https://hg.mozilla.org/mozilla-central/rev/41a6c5fd2986
https://hg.mozilla.org/mozilla-central/rev/409e2d357905
https://hg.mozilla.org/mozilla-central/rev/16469cbbbabe
https://hg.mozilla.org/mozilla-central/rev/ec471701d5ff
https://hg.mozilla.org/mozilla-central/rev/f476926ccb11
https://hg.mozilla.org/mozilla-central/rev/ebfb9b899c4d
https://hg.mozilla.org/mozilla-central/rev/87b3efc6b594
https://hg.mozilla.org/mozilla-central/rev/d092f5abddc8