Closed Bug 1634281 Opened 2 years ago Closed 7 months ago

Unify various ns*Hashtable classes that are subclasses of nsBaseHashtable

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
Tracking Status
firefox88 --- fixed

People

(Reporter: sg, Assigned: sg)

References

Details

Attachments

(14 files, 6 obsolete files)

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

There are various hashtable classes including:

  • nsDataHashtable
  • nsClassHashtable
  • nsInterfaceHashtable
  • nsJSThingHashtable
  • nsRefPtrHashTable
    which could probably be unified into a single nsHashtable<Key, Value> class template using some mechanism properly deducing the appropriate hash key type.
Assignee: nobody → sgiesecke

Before diving into changes it would be good to take a look at what unifying these would mean for usability, how big of a code impact it would have, what the purpose of each is and any specializations we've added.

For example for nsClassHashtable we accept a raw pointer as an argument and then take ownership by storing in a UniquePtr, we then allow retrieval of the raw ptr. Switching to only accepting/exposing UniquePtr could be a rather large change, it would be interesting to know what the impact would be.

Status: NEW → ASSIGNED

Depends on D105961

Depends on: 1693267

The only difference between nsRefPtrHashtable and nsInterfaceHashtable was
that the former enforced explicit refcounting also with InsertOrUpdate.
This patch adapts the uses of nsInterfaceHashtable::InsertOrUpdate to also
make this explicit.

Depends on D105971

(In reply to Eric Rahm [:erahm] from comment #1)

Before diving into changes it would be good to take a look at what unifying these would mean for usability, how big of a code impact it would have, what the purpose of each is and any specializations we've added.

The intention of this specific bug is not so much to change the API the different variants offer, but to make them available as a single type alias, and select the right implementation. Making the same API available for all variants is probably not the way to go. Specifically nsRefPtrHashtable and nsInterfaceHashtable require explicit refcounting and distinguish strong and weak lookups intentionally.

For example for nsClassHashtable we accept a raw pointer as an argument and then take ownership by storing in a UniquePtr, we then allow retrieval of the raw ptr. Switching to only accepting/exposing UniquePtr could be a rather large change, it would be interesting to know what the impact would be.

This change has already been made outside of this bug. A raw pointer is now exposed in the Get member functions, storing a raw pointer requires explicit conversion to UniquePtr.

Summary: Unify various hashtable classes → Unify various ns*Hashtable classes that are subclasses of nsBaseHashtable
Attachment #9204570 - Attachment description: Bug 1634281 - Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers
Attachment #9204571 - Attachment description: Bug 1634281 - Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers
Attachment #9204572 - Attachment description: Bug 1634281 - Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers
Attachment #9204643 - Attachment description: Bug 1634281 - Use nsUniversalHashtable instead of nsDataHashtable. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsUniversalHashtable instead of nsDataHashtable. r=#xpcom-reviewers
Attachment #9204814 - Attachment description: Bug 1634281 - Remove nsDataHashtable. r=#xpcom-reviewers → Bug 1634281 - WIP: Remove nsDataHashtable. r=#xpcom-reviewers
Attachment #9204815 - Attachment description: Bug 1634281 - Use implicit hash key class deduction with former nsDataHashtable uses. r=#xpcom-reviewers → Bug 1634281 - WIP: Use implicit hash key class deduction with former nsDataHashtable uses. r=#xpcom-reviewers
Attachment #9204817 - Attachment description: Bug 1634281 - Use nsUniversalHashtable instead of nsRefPtrHashtable. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsUniversalHashtable instead of nsRefPtrHashtable. r=#xpcom-reviewers
Attachment #9204817 - Attachment is obsolete: true
Attachment #9204569 - Attachment description: Bug 1634281 - Add nsUniversalHashtable. r=#xpcom-reviewers → Bug 1634281 - WIP: Add nsTHashMap. r=#xpcom-reviewers
Attachment #9204570 - Attachment description: Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers
Attachment #9204571 - Attachment description: Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers
Attachment #9204572 - Attachment description: Bug 1634281 - WIP: Use nsUniversalHashtable for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers
Attachment #9204643 - Attachment description: Bug 1634281 - WIP: Use nsUniversalHashtable instead of nsDataHashtable. r=#xpcom-reviewers → Bug 1634281 - WIP: Use nsTHashMap instead of nsDataHashtable. r=#xpcom-reviewers
Attachment #9204569 - Attachment description: Bug 1634281 - WIP: Add nsTHashMap. r=#xpcom-reviewers → Bug 1634281 - Add nsTHashMap. r=#xpcom-reviewers
Attachment #9205456 - Attachment description: Bug 1634281 - Use nsBaseHashtable::GetOrInsertNew(Derived) where possible. r=#xpcom-reviewers → Bug 1634281 - Use nsBaseHashtable::GetOrInsertNew where possible. r=#xpcom-reviewers
Keywords: leave-open
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8074635f949d
Pull up GetOrInsertNew to nsBaseHashtable and support other smart pointer types. r=xpcom-reviewers,nika
Attachment #9204815 - Attachment is obsolete: true
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea82d6881480
Remove unused nsDataHashtable.h includes. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/5d7f1cb9c48a
Use nsBaseHashtable::GetOrInsertNew where possible. r=xpcom-reviewers,nika
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9082910f556e
Add nsHashtablesFwd.h and remove all other forward declarations for hashtables. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/2206866db9c3
Make nsDataHashtable a type alias. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/80f6324d2e6b
Merge nsRefPtrHashtable and nsInterfaceHashtable into nsRefCountedHashtable. r=xpcom-reviewers,necko-reviewers,nika
Attachment #9204570 - Attachment description: Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers → Bug 1634281 - Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=#xpcom-reviewers
Attachment #9204571 - Attachment description: Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers → Bug 1634281 - Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=#xpcom-reviewers
Attachment #9204572 - Attachment description: Bug 1634281 - WIP: Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers → Bug 1634281 - Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=#xpcom-reviewers
Attachment #9204643 - Attachment description: Bug 1634281 - WIP: Use nsTHashMap instead of nsDataHashtable. r=#xpcom-reviewers → Bug 1634281 - Use nsTHashMap instead of nsDataHashtable. r=#xpcom-reviewers
Attachment #9204814 - Attachment description: Bug 1634281 - WIP: Remove nsDataHashtable. r=#xpcom-reviewers → Bug 1634281 - Remove nsDataHashtable. r=#xpcom-reviewers
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06f2efafd9a1
Add nsTHashMap. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/9a8bf872ef93
Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with RefPtr data type. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/8936c72646ff
Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with nsCOMPtr data type. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/ee2da01a35a3
Use nsTHashMap for nsBaseHashtable/nsDataHashtable uses with UniquePtr data type. r=xpcom-reviewers,nika
Attachment #9205445 - Attachment is obsolete: true
Attachment #9207830 - Attachment is obsolete: true
Attachment #9207832 - Attachment is obsolete: true
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b4f47796625
Use nsTHashMap instead of nsDataHashtable. r=xpcom-reviewers,necko-reviewers,jgilbert,nika,valentin
https://hg.mozilla.org/integration/autoland/rev/d5f05e5ae9eb
Remove nsDataHashtable. r=xpcom-reviewers,nika
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5166f17b44a
Use (STL) algorithms instead of custom hashtable iteration where possible in dom/localstorage. r=dom-storage-reviewers,janv
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Attachment #9207831 - Attachment is obsolete: true
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/761cc501b90a
Use (STL) algorithms instead of custom hashtable iteration where possible in dom/animation. r=hiro
You need to log in before you can comment on or make changes to this bug.