Closed Bug 1688833 Opened 2 years ago Closed 2 years ago

Migrate nsBaseHashtable::LookupForAdd to nsBaseHashtable::WithEntryHandle

Categories

(Core :: XPCOM, task, P2)

task

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: janv, Assigned: sg)

References

Details

Attachments

(31 files)

47 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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:

Summary: Merge the old and the new implementation of nsBaseHashtable::LookupForAdd → Merge the old and the new API for BaseHashtable::LookupForAdd
Summary: Merge the old and the new API for BaseHashtable::LookupForAdd → Merge the old and the new API for BaseHashtable::LookupForAdd (WithEntryHandle)
Summary: Merge the old and the new API for BaseHashtable::LookupForAdd (WithEntryHandle) → Merge the old and the new API for nsBaseHashtable::LookupForAdd (WithEntryHandle)
Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Summary: Merge the old and the new API for nsBaseHashtable::LookupForAdd (WithEntryHandle) → Migrate nsBaseHashtable::LookupForAdd to nsBaseHashtable::WithEntryHandle

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

Adds unit tests.

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

Flags: needinfo?(sgiesecke)

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.

Flags: needinfo?(sgiesecke)
Pushed by sgiesecke@mozilla.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Depends on: 1371094
Blocks: 1691894
See Also: → 1372333
Duplicate of this bug: 1277533
You need to log in before you can comment on or make changes to this bug.