Closed Bug 1121202 Opened 5 years ago Closed 5 years ago

remove PL_DHashTableOperate

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: froydnj, Assigned: michael, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 3 obsolete files)

We have these shiny new PL_DHashTable{Add,Remove,Lookup} functions; we don't need the *Operate version anymore.
Depends on: 1120262, 1118024
Michael, want to take this one? :)
Flags: needinfo?(michael)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Michael, want to take this one? :)

Sure, thanks.
Assignee: nobody → michael
Flags: needinfo?(michael)
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8548778 - Flags: review?(nfroyd)
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.
Flags: needinfo?(Pidgeot18)
Comment on attachment 8548778 [details] [diff] [review]
Patch

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

Thank you for adjusting the comments, too!
Attachment #8548778 - Flags: review?(nfroyd) → review+
Depends on: 1121482
Flags: needinfo?(Pidgeot18)
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}.
(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.
(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.)
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.
Attached patch Patch (obsolete) — Splinter Review
I've updated the comments to replace all references to PL_DHashTableOperate() with their appropriate replacements.
Attachment #8548778 - Attachment is obsolete: true
Attachment #8549377 - Flags: review?(nfroyd)
Attachment #8549377 - Flags: review?(n.nethercote)
Attached patch Patch Part 2 (obsolete) — Splinter Review
And here's a patch which adds Lookup, Add, and Remove methods to PLDHashTable and makes Operate a private method.
Attachment #8549389 - Flags: review?(nfroyd)
Attachment #8549389 - Flags: review?(n.nethercote)
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.
Attachment #8549377 - Flags: review?(nfroyd)
Attachment #8549377 - Flags: review?(n.nethercote)
Attachment #8549377 - Flags: review+
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.
Attachment #8549389 - Flags: review?(nfroyd)
Attachment #8549389 - Flags: review?(n.nethercote)
Attachment #8549389 - Flags: review+
Attached patch PatchSplinter Review
Thanks, Nicholas; I've fixed the formatting issue you noted.
Attachment #8549377 - Attachment is obsolete: true
Attachment #8549955 - Flags: review?(n.nethercote)
Attached patch Patch Part 2Splinter Review
Thanks, Nicholas; I've marked the Lookup, Add, and Remove methods as MOZ_ALWAYS_INLINE as you suggested.
Attachment #8549389 - Attachment is obsolete: true
Attachment #8549956 - Flags: review?(n.nethercote)
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?
Attachment #8549955 - Flags: review?(n.nethercote) → review+
Attachment #8549956 - Flags: review?(n.nethercote) → review+
(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.
> 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.
Flags: needinfo?(michael)
Here are the results from the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ceb40eb9d3
Flags: needinfo?(michael)
Keywords: checkin-needed
Depends on: 1126859
No longer depends on: 1126859
You need to log in before you can comment on or make changes to this bug.