1246846 - Avoid nsTHashtable::RawRemoveEntry() where possible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Avoid nsTHashtable::RawRemoveEntry() in dom/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Avoid nsTHashtable::RawRemoveEntry() in nsPermissionManager

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Avoid nsTHashtable::RawRemoveEntry() in gfxFontconfigUtils

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Avoid nsTHashtable::RawRemoveEntry() in FramePropertyTable

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8717293 [details] [diff] [review]
(part 4) - Avoid nsTHashtable::RawRemoveEntry() in FramePropertyTable

r=me

Comment 7

[Nicholas Nethercote [inactive]]

> 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.

Comment 8

[Mike Connor [:mconnor]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Nicholas Nethercote [inactive]]

> 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.

Comment 11

[William Chen [: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

Comment 12

[William Chen [:wchen]]

Looks like it was added in bug 572614 in this patch: http://hg.mozilla.org/mozilla-central/rev/3d65cff5c900

Comment 13

[Nicholas Nethercote [inactive]]

At least one of these changes is no good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ed6a90a513&selectedJob=16532103

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Document need for nsTHashtable::RawRemoveEntry() in FramePropertyTable

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.

Comment 15

[Nicholas Nethercote [inactive]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Can't we still RemoveEntry in RemoveInternal, since we immediately null out mLastEntry?

Comment 17

[Nicholas Nethercote [inactive]]

(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.)

Comment 18

[Nicholas Nethercote [inactive]]

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.

Comment 19

[Nicholas Nethercote [inactive]]

> > 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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c11d80f6718
https://hg.mozilla.org/mozilla-central/rev/55e42a330e4a
https://hg.mozilla.org/mozilla-central/rev/8e0f6b61081d
https://hg.mozilla.org/mozilla-central/rev/092510c19ca5