Migrate nsBaseHashtable::LookupForAdd to nsBaseHashtable::WithEntryHandle
Categories
(Core :: XPCOM, task, P2)
Tracking
()
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 | |
Bug 1688833 - Migrate LookupForAdd to WithEntryHandle in toolkit/components/antitracking. r=timhuang
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:
- Implement
EntryHandle::OrInsertFrom
- Check the semantics of
PLDHashTable::EntryHandle::operator bool
andnsBaseHashtable::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 unsafeEntryHandle::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)
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Implements the missing handle functions (OrInsertFrom), and harmonizes functions
to return a reference to the data.
Adds unit tests.
Assignee | ||
Comment 2•4 years ago
|
||
Depends on D99780
Assignee | ||
Comment 3•4 years ago
|
||
Depends on D104191
Assignee | ||
Comment 4•4 years ago
|
||
Depends on D104192
Assignee | ||
Comment 5•4 years ago
|
||
Depends on D104193
Assignee | ||
Comment 6•4 years ago
|
||
Depends on D104194
Assignee | ||
Comment 7•4 years ago
|
||
Depends on D104195
Assignee | ||
Comment 8•4 years ago
|
||
Depends on D104196
Assignee | ||
Comment 9•4 years ago
|
||
Depends on D104197
Assignee | ||
Comment 10•4 years ago
|
||
Depends on D104200
Assignee | ||
Comment 11•4 years ago
|
||
Depends on D104201
Assignee | ||
Comment 12•4 years ago
|
||
Depends on D104202
Comment 13•4 years ago
|
||
What is the reasoning for this new API? It seems much uglier / harder to read than the current one...
Assignee | ||
Comment 14•4 years ago
|
||
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.
Assignee | ||
Comment 15•4 years ago
|
||
Depends on D104203
Assignee | ||
Comment 16•4 years ago
|
||
Depends on D104215
Assignee | ||
Comment 17•4 years ago
|
||
Depends on D104216
Assignee | ||
Comment 18•4 years ago
|
||
Depends on D104217
Assignee | ||
Comment 19•4 years ago
|
||
Depends on D104218
Assignee | ||
Comment 20•4 years ago
|
||
Depends on D104219
Assignee | ||
Comment 21•4 years ago
|
||
Depends on D104220
Assignee | ||
Comment 22•4 years ago
|
||
Depends on D104221
Assignee | ||
Comment 23•4 years ago
|
||
Depends on D104222
Assignee | ||
Comment 24•4 years ago
|
||
Depends on D104223
Assignee | ||
Comment 25•4 years ago
|
||
Depends on D104224
Assignee | ||
Comment 26•4 years ago
|
||
Depends on D104225
Assignee | ||
Comment 27•4 years ago
|
||
Depends on D104226
Assignee | ||
Comment 28•4 years ago
|
||
Depends on D104227
Assignee | ||
Comment 29•4 years ago
|
||
Depends on D104228
Assignee | ||
Comment 30•4 years ago
|
||
Depends on D104229
Assignee | ||
Comment 31•4 years ago
|
||
Depends on D104230
Assignee | ||
Comment 32•4 years ago
|
||
Depends on D104231
Assignee | ||
Comment 33•4 years ago
|
||
Depends on D104232
Comment 34•4 years ago
|
||
Comment 35•4 years ago
|
||
bugherder |
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
Description
•