Add to PLDHashTable the ability to remove an already-found entry

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

3 years ago
(This is a slightly modified version of bug 848609.)

A very common use case for a hash table is this:

(1) lookup item
(2) do something with it
(3) remove it

With pldhash, for step (3) you either have to Remove(), which repeats the search, or use RawRemove(), which does not subsequently shrink the table if its capacity gets too low.

This bug is about adding a new function that takes an already found entry (like RawRemove()) but also shrinks the table afterwards if appropriate (like Remove()). And then using it when appropriate.
(Assignee)

Comment 1

3 years ago
Created attachment 8658009 [details] [diff] [review]
(part 1) - Add PLDHashTable::RemoveEntry()

This patch also consolidates the shrink handling so it's now entirely within
ShrinkIfAppropriate().
Attachment #8658009 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8658010 [details] [diff] [review]
(part 2) - Avoid PL_DHashTableRawRemove() in nsLoadGroup

This assumes that it's safe to possibly shrink the table after the removal,
i.e. there are no surprising subtleties with how this table is managed.
Attachment #8658010 - Flags: review?(mcmanus)
(Assignee)

Comment 3

3 years ago
Created attachment 8658011 [details] [diff] [review]
(part 3) - Avoid PL_DHashTableRawRemove() in nsDocument

This assumes that it's safe to possibly shrink the table after the removal,
i.e. there are no surprising subtleties with how this table is managed.
Attachment #8658011 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Created attachment 8658012 [details] [diff] [review]
(part 4) - Avoid PL_DHashTableRawRemove() in nsPropertyTable

This assumes that it's safe to possibly shrink the table after the removal,
i.e. there are no surprising subtleties with how this table is managed.
Attachment #8658012 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

3 years ago
Created attachment 8658014 [details] [diff] [review]
(part 5) - Use PLDHashTable::RemoveEntry() in nsSecureBrowserUIImpl

This avoids repeating the hash table search in order to remove the entry.
Attachment #8658014 - Flags: review?(dkeeler)
(Assignee)

Comment 6

3 years ago
Created attachment 8658015 [details] [diff] [review]
(part 6) - Use PLDHashTable::RemoveEntry() in XULDocument

This avoids repeating the hash table search in order to remove the entry.
Attachment #8658015 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

3 years ago
Created attachment 8658016 [details] [diff] [review]
(part 7) - Use PLDHashTable::RemoveEntry() in prefs code

This avoids repeating the hash table search in order to remove the entry.
Attachment #8658016 - Flags: review?(nfroyd)
(Assignee)

Comment 8

3 years ago
Created attachment 8658017 [details] [diff] [review]
(part 8) - Use PLDHashTable::RemoveEntry() in the cycle collector

This avoids repeating the hash table search in order to remove a CC graph
entry, which is good because it's a common operation.
Attachment #8658017 - Flags: review?(continuation)
Blocks: 1202670
Attachment #8658017 - Flags: review?(continuation) → review+
Attachment #8658010 - Flags: review?(mcmanus) → review+
Attachment #8658014 - Flags: review?(dkeeler) → review+
Attachment #8658009 - Flags: review?(nfroyd) → review+
Attachment #8658016 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

3 years ago
Blocks: 1128407
Comment on attachment 8658011 [details] [diff] [review]
(part 3) - Avoid PL_DHashTableRawRemove() in nsDocument

r=me
Attachment #8658011 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658012 [details] [diff] [review]
(part 4) - Avoid PL_DHashTableRawRemove() in nsPropertyTable

r=me
Attachment #8658012 - Flags: review?(bzbarsky) → review+
Comment on attachment 8658015 [details] [diff] [review]
(part 6) - Use PLDHashTable::RemoveEntry() in XULDocument

r=me
Attachment #8658015 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

3 years ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cda97f7f4516

(Ignore all the red W tests in that push. There was some problem with them when I did the push.)
(Assignee)

Updated

3 years ago
Blocks: 1209351
You need to log in before you can comment on or make changes to this bug.