Closed Bug 1246846 Opened 6 years ago Closed 6 years ago

Avoid nsTHashtable::RawRemoveEntry() where possible

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(4 files, 1 obsolete file)

nsTHashtable::RawRemoveEntry() does not shrink the hash table after removal. It's best used only in unusual cases where entry stability is important. This bug is about converting the non-unusual cases to instead use nsTHashtable::RemoveEntry().

Bug 1202526 did a similar thing with PLDHashTable.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8717290 [details] [diff] [review]
(part 1) - Avoid nsTHashtable::RawRemoveEntry() in dom/

Did you happen to look up the history of why nsDocument::RemoveFromIdTable does what it does?  That one could almost theoretically be based on some sort of performance measurements; that's reasonably hot code...  Or does it just predate the existence of a RemoveEntry that doesn't need a separate lookup?
Flags: needinfo?(n.nethercote)
Comment on attachment 8717293 [details] [diff] [review]
(part 4) - Avoid nsTHashtable::RawRemoveEntry() in FramePropertyTable

r=me
Attachment #8717293 - Flags: review?(bzbarsky) → review+
> Did you happen to look up the history of why nsDocument::RemoveFromIdTable
> does what it does?  That one could almost theoretically be based on some
> sort of performance measurements; that's reasonably hot code...  Or does it
> just predate the existence of a RemoveEntry that doesn't need a separate
> lookup?

Right: so until fairly recently there were two possible reasons for using RawRemoveEntry(): (a) speed and (b) stability (i.e. no possibility of resizing). The goal of this bug is to convert only the speed cases. I've modified cases where we do a RawRemoveEntry() very shortly after a GetEntry(), which seem likely to be speed cases, but I was hoping that reviewers would be able to tell me if I'm wrong...

To answer your question, I haven't looked up the history.
Flags: needinfo?(n.nethercote)
Comment on attachment 8717291 [details] [diff] [review]
(part 2) - Avoid nsTHashtable::RawRemoveEntry() in nsPermissionManager

If this was a speed optimization, it was probably a premature one.  Permission writes really aren't that common under typical usage, removals even less so.
Attachment #8717291 - Flags: review?(mconnor) → review+
Attachment #8717292 - Flags: review?(jfkthame) → review+
Summary: Add to PLDHashTable the ability to remove an already-found entry → Avoid nsTHashtable::RawRemoveEntry() where possible
Comment on attachment 8717290 [details] [diff] [review]
(part 1) - Avoid nsTHashtable::RawRemoveEntry() in dom/

OK.  Pretty sure this was just trying to avoid a second lookup, not trying to optimize out the hashtable resize.  r=me
Attachment #8717290 - Flags: review?(bzbarsky) → review+
> To answer your question, I haven't looked up the history.

I just did. wchen added it in bug 806506. wchen: are you able to confirm that the use of RawRemoveEntry() in nsDocument::RemoveFromIdTable() was for speed, rather than because resizing the table is possible dangerous? (See comment 7 for more details.) Thank you.
Flags: needinfo?(wchen)
(In reply to Nicholas Nethercote [:njn] from comment #10)
> > To answer your question, I haven't looked up the history.
> 
> I just did. wchen added it in bug 806506. wchen: are you able to confirm
> that the use of RawRemoveEntry() in nsDocument::RemoveFromIdTable() was for
> speed, rather than because resizing the table is possible dangerous? (See
> comment 7 for more details.) Thank you.

My patch didn't add RawRemoveEntry in nsDocument::RemoveFromIdTable. It's been there since hg@1
Flags: needinfo?(wchen)
This is the patch that caused the try failures. I'm about 90% sure that its
mLastEntry that's the reason why RawRemoveEntry() is needed -- it's certainly
the only possibility I can see.
Attachment #8717773 - Flags: review?(bzbarsky)
Attachment #8717293 - Attachment is obsolete: true
Comment on attachment 8717773 [details] [diff] [review]
(part 4) - Document need for nsTHashtable::RawRemoveEntry() in FramePropertyTable

Review of attachment 8717773 [details] [diff] [review]:
-----------------------------------------------------------------

AFAICT roc originally wrote this code so I'll ask him to confirm if I've explained this properly.
Attachment #8717773 - Flags: review?(bzbarsky) → review?(roc)
Can't we still RemoveEntry in RemoveInternal, since we immediately null out mLastEntry?
(In reply to Boris Zbarsky [:bz] from comment #16)
> Can't we still RemoveEntry in RemoveInternal, since we immediately null out
> mLastEntry?

I'll try it. (I did try nulling mLastEntry after the other RawRemoveEntry(), but that still caused problems.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c11d80f6718c3b41c9807e7eb1c195f6e377e59
Bug 1246846 (part 1) - Avoid nsTHashtable::RawRemoveEntry() in dom/. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/55e42a330e4a24126fbd894034b9b34d66621168
Bug 1246846 (part 2) - Avoid nsTHashtable::RawRemoveEntry() in nsPermissionManager. r=mconnor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0f6b61081d0cd8547d3dec03ea54aed7303f1e
Bug 1246846 (part 3) - Avoid nsTHashtable::RawRemoveEntry() in gfxFontconfigUtils. r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/092510c19ca5514165e7493c75468f348ad053bc
Bug 1246846 (part 4) - Avoid nsTHashtable::RawRemoveEntry() in FramePropertyTable. r=roc.
> > Can't we still RemoveEntry in RemoveInternal, since we immediately null out mLastEntry?
> 
> I'll try it.

This gave a good result on try, so I landed it.
You need to log in before you can comment on or make changes to this bug.