1121202 - remove PL_DHashTableOperate

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

We have these shiny new PL_DHashTable{Add,Remove,Lookup} functions; we don't need the *Operate version anymore.

Comment 1

[Nicholas Nethercote [inactive]]

Michael, want to take this one? :)

Comment 2

[Michael Pruett]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Michael, want to take this one? :)

Sure, thanks.

Comment 3

[Michael Pruett]

Attachment: Patch

This patch removes the PL_DHashTableOperate function, which is no longer used.

Here are the results from the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f6125e91a6

Comment 4

[Andrew McCreight [:mccr8]]

jcranmer: this will break comm-central. See bug 1118024 for the kind of conversion that needs to be done. It should be easy, and the resulting patch can be landed right now.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8548778 [details] [diff] [review]
Patch

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

Thank you for adjusting the comments, too!

Comment 6

[Nicholas Nethercote [inactive]]

This is good, but doesn't go as far as it could. You could also split up PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}.

Comment 7

[Michael Pruett]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This is good, but doesn't go as far as it could. You could also split up
> PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}.

Agreed. I will make similar changes to the PLDHashTable class in a separate bug.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This is good, but doesn't go as far as it could. You could also split up
> PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}.

I would suggest keeping Operate, as it keeps all the hashing logic in one place; we can rely on the compiler always inlining it.  (Moving to a more class-based scheme for this would let us make Operate() private, of course.)

Comment 9

[Nicholas Nethercote [inactive]]

I think there are also some more references to PL_DHashTableOperate in comments in pldhash.h that your patch doesn't remove. And there's a comment that mentions it in dom/plugins/base/nsJSNPRuntime.cpp.

Comment 10

[Michael Pruett]

Attachment: Patch

I've updated the comments to replace all references to PL_DHashTableOperate() with their appropriate replacements.

Comment 11

[Michael Pruett]

Attachment: Patch Part 2

And here's a patch which adds Lookup, Add, and Remove methods to PLDHashTable and makes Operate a private method.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8549377 [details] [diff] [review]
Patch

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

I don't think this needs two reviews :) Thanks.

::: xpcom/glue/pldhash.h
@@ +145,4 @@
>   * aside, e.g., to avoid copying live entries into an array of the entry type.
>   * Copying entry pointers is cheaper, and safe so long as the caller of such a
>   * "stable" Enumerate doesn't use the set-aside pointers after any call either
> + * to PL_DHashTableAdd or PL_DHashTableRemove, or to an "unstable" form of Enumerate, which might

Nit: this line is now over 80 chars.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8549389 [details] [diff] [review]
Patch Part 2

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

::: xpcom/glue/pldhash.cpp
@@ +664,5 @@
> +PLDHashEntryHdr*
> +PLDHashTable::Lookup(const void* aKey)
> +{
> +  return Operate(aKey, PL_DHASH_LOOKUP);
> +}

Nit: these should have MOZ_ALWAYS_INLINE on them like Init(), Operate(), etc.

Comment 14

[Michael Pruett]

Attachment: Patch

Thanks, Nicholas; I've fixed the formatting issue you noted.

Comment 15

[Michael Pruett]

Attachment: Patch Part 2

Thanks, Nicholas; I've marked the Lookup, Add, and Remove methods as MOZ_ALWAYS_INLINE as you suggested.

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8549955 [details] [diff] [review]
Patch

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

A tip: when somebody gives r+ and has some comments it typically indicates that the changes required are small enough that the reviewer trusts the author to do them without a follow-up review. (If they want a follow-up review they'll typically give r- or f+ and explicitly ask for a follow-up.) But this is not obvious and if in doubt it doesn't hurt to ask again. Thanks.

Do you need someone else to land these for you?

Comment 17

[Michael Pruett]

(In reply to Nicholas Nethercote [:njn] from comment #16) 
> A tip: when somebody gives r+ and has some comments it typically indicates
> that the changes required are small enough that the reviewer trusts the
> author to do them without a follow-up review. (If they want a follow-up
> review they'll typically give r- or f+ and explicitly ask for a follow-up.)
> But this is not obvious and if in doubt it doesn't hurt to ask again. Thanks.
> 
> Do you need someone else to land these for you?

Thanks for your guidance, Nicholas. I plan to add the checkin-needed keyword once bug 1121482 has been resolved.

Comment 18

[Nicholas Nethercote [inactive]]

> I plan to add the checkin-needed keyword once bug 1121482 has been resolved.

Generally sheriffs are only happy to check-in a patch that has had a try run, so that obviously broken stuff doesn't get landed. (See step 6 in https://developer.mozilla.org/en/docs/Introduction.)

Do you have try access? If not, I can do a run for you, but you might like to consider requesting it so you can do it yourself in the future. Here are the relevant docs:

https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
https://www.mozilla.org/en-US/about/governance/policies/commit/

I'd be happy to vouch for you.

Bug 1007046 is an example bug where somebody else got L1 access, which might be helpful.

Comment 19

[Michael Pruett]

Here are the results from the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ceb40eb9d3

Comment 20

[Nicholas Nethercote [inactive]]

Great! Thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a53fee1e25
https://hg.mozilla.org/integration/mozilla-inbound/rev/d614944d001c

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4a53fee1e25
https://hg.mozilla.org/mozilla-central/rev/d614944d001c