Implement nsBaseHashtable::LookupForAdd

RESOLVED FIXED in Firefox 56

Status

()

Core
XPCOM
P2
enhancement
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 months ago
I need a LookupForAdd() method to fix some perf issues in mozilla::dom::ImageTracker
(Assignee)

Updated

3 months ago
Blocks: 1371095
Thank you for doing this!
(Assignee)

Comment 2

2 months ago
Created attachment 8876105 [details] [diff] [review]
Introduce a nsBaseHashtable::LookupForAdd() method

This is basically a copy of the nsClassHashtable version, but with
two differences:
1. nsClassHashtable treats null values for an existing entry the same
   as a non-existing entry:
http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/xpcom/ds/nsClassHashtable.h#99,106
   We can't do that here since we don't know if UserDataType is a pointer
   type or not.  Instead, I'm checking Count() before and after the call
   to PutEntry() to determine of an entry was added.  I considered adding
   new API to ferry out this state all the way from PLDHashTable, but it
   doesn't seem worth it to me.  Checking Count() should be as fast and
   accurate.

2. I'm adding a Data() method for easy access to the value in the entry.
   This is needed in multiple places, for example in ImageTracker which
   stores a counter that needs to incremented. (see bug 1371095)

I kept nsClassHashtable::LookupForAdd for now since it treats null values
differently.  I don't know if that's intentional though.  If not, we can
remove it and let it inherit this instead.
Attachment #8876105 - Flags: review?(nfroyd)
Comment on attachment 8876105 [details] [diff] [review]
Introduce a nsBaseHashtable::LookupForAdd() method

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

Checking Count() is probably about as fast as you can get; trying to do it inside PLDHashTable would require altering Add() or similar, which we've found to be very perf-sensitive, or adding an entirely new API...  Maybe we could come up with some inventive specialization that would enable us to use null pointer checks (which should be faster) on pointer-like values, and Count() where that's not available?

Can you add a comment in nsClassHashtable::LookupForAdd about why we're keeping that?  And, come to think of it, if we're going to have a separate nsClassHashtable method, why shouldn't we have one for nsRefPtr/InterfaceHashtable, which behave approximately the same way?

Not opposed to adding this at all, I just think there are a couple more things to get nailed down before we commit this.
Attachment #8876105 - Flags: review?(nfroyd) → review-
(Assignee)

Comment 4

2 months ago
> why shouldn't we have one for nsRefPtr/InterfaceHashtable, which behave approximately the same way?

Good point.  I'm starting to think that the current behavior is simply a bug.
I see no reason why LookupForAdd should have different behavior from Get/GetOrInsert etc.
AFIAK, all three of these classes supports inserting nullptr as a value - it's rather odd
that you can't retrieve that value using nsClassHashtable::LookupForAdd.

Let's see if anything breaks by removing it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74b5c21c8daa8167f0d15315554bfa331608880f
(Assignee)

Comment 5

2 months ago
Created attachment 8876391 [details] [diff] [review]
Move the nsClassHashtable::LookupForAdd() method to nsBaseHashtable

(I also changed nsClassHashtable::LookupOrAdd to make it consistent.)
Attachment #8876105 - Attachment is obsolete: true
Attachment #8876391 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Blocks: 1371952
(Assignee)

Comment 6

2 months ago
Created attachment 8876454 [details] [diff] [review]
Move the nsClassHashtable::LookupForAdd() method to nsBaseHashtable

A minor tweak to the assertion in the dtor:
  MOZ_ASSERT(mEntry.mData, "Entry should have been added by now");
because it makes no sense unless mData is a pointer and we use
null to indicate "not set" (which is dubious in the first place).

I added a DEBUG bool instead to track whether OrInsert was called
in the non-existing entry case.
Attachment #8876391 - Attachment is obsolete: true
Attachment #8876391 - Flags: review?(nfroyd)
Attachment #8876454 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Blocks: 1372007
(Assignee)

Updated

2 months ago
Blocks: 1372008
(Assignee)

Updated

2 months ago
Blocks: 1372013
(Assignee)

Updated

2 months ago
No longer blocks: 1372013
(Assignee)

Updated

2 months ago
Blocks: 1372022
(Assignee)

Updated

2 months ago
Blocks: 1372025
(Assignee)

Updated

2 months ago
Whiteboard: [qf]
(Assignee)

Updated

2 months ago
Blocks: 1372048
(Assignee)

Updated

2 months ago
Blocks: 1372049
(Assignee)

Updated

2 months ago
Blocks: 1372262
(Assignee)

Updated

2 months ago
Blocks: 1372274
Comment on attachment 8876454 [details] [diff] [review]
Move the nsClassHashtable::LookupForAdd() method to nsBaseHashtable

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

r=me with the below test added somewhere appropriate under xpcom/tests/gtest/.

::: xpcom/ds/nsBaseHashtable.h
@@ +234,5 @@
> +      MOZ_ASSERT(mTableGeneration == mTable.GetGeneration());
> +      if (!mExistingEntry) {
> +        mEntry.mData = func();
> +#ifdef DEBUG
> +        mDidInitNewEntry = true;

We should have an explicit test that:

auto e = ht.LookupForAdd(...);
T& t = e.OrInsert(...);
if (!e) {
}

works, i.e. that the boolean-ness of EntryPtr is not tied to stuff being present in the entry or not.  I saw this pattern in a patch that depends on this code and it threw me a little...and it wouldn't be any fun to track down if somebody accidentally changed said behavior.
Attachment #8876454 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

2 months ago
Created attachment 8876806 [details] [diff] [review]
part 2 - Add some tests for the LookupForAdd/OrInsert/LookupRemoveIf methods

OK, let me know if I missed something.
Attachment #8876806 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Blocks: 1372323
(Assignee)

Updated

2 months ago
Blocks: 1372327
(Assignee)

Updated

2 months ago
Blocks: 1372342
(Assignee)

Updated

2 months ago
Blocks: 1372349
(Assignee)

Updated

2 months ago
Blocks: 1372368
Comment on attachment 8876806 [details] [diff] [review]
part 2 - Add some tests for the LookupForAdd/OrInsert/LookupRemoveIf methods

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

Thank you!
Attachment #8876806 - Flags: review?(nfroyd) → review+

Comment 10

2 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b01e6d83f2c
part 1 - Move the nsClassHashtable::LookupForAdd() method to nsBaseHashtable.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2197af9bfc
part 2 - Add some tests for the LookupForAdd/OrInsert/LookupRemoveIf methods.  r=froydnj

Comment 11

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b01e6d83f2c
https://hg.mozilla.org/mozilla-central/rev/fb2197af9bfc
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 months ago
Depends on: 1374089
(Assignee)

Updated

2 months ago
Blocks: 1374126
(Assignee)

Updated

2 months ago
Blocks: 1374089
No longer depends on: 1374089
(Assignee)

Updated

2 months ago
Blocks: 1375688
(Assignee)

Updated

2 months ago
Blocks: 1375696
(Assignee)

Updated

2 months ago
Blocks: 1376127
(Assignee)

Updated

2 months ago
Blocks: 1376469
(Assignee)

Updated

2 months ago
Blocks: 1376476
(Assignee)

Updated

2 months ago
Blocks: 1376483
(Assignee)

Updated

2 months ago
Blocks: 1376487
You need to log in before you can comment on or make changes to this bug.