Closed Bug 1688833 Opened 4 years ago Closed 4 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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Depends on: 1371094
Blocks: 1691894
See Also: → 1372333
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: