Closed Bug 1427373 Opened 6 years ago Closed 6 years ago

Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

Categories

(Core :: Networking: DNS, enhancement, P2)

Product:
Core
Component:
Networking: DNS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to release it
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
      No description provided.
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review215242

Yeah this looks great. I think maybe there are a few merge conflicts?

::: netwerk/dns/nsHostResolver.cpp:534
(Diff revision 1)
>          PRCList *node = mEvictionQ.next;
>          while (node != &mEvictionQ) {
>              nsHostRecord *rec = static_cast<nsHostRecord *>(node);
>              node = node->next;
>              PR_REMOVE_AND_INIT_LINK(rec);
> -            mDB.Remove(static_cast<nsHostKey *>(rec));
> +            mRecordDB.Remove(*(nsHostKey *) rec);

static_cast?

::: netwerk/dns/nsHostResolver.cpp:1262
(Diff revision 1)
>              else {
>                  // remove first element on mEvictionQ
>                  nsHostRecord *head =
>                      static_cast<nsHostRecord *>(PR_LIST_HEAD(&mEvictionQ));
>                  PR_REMOVE_AND_INIT_LINK(head);
> -                mDB.Remove(static_cast<nsHostKey *>(head));
> +                mRecordDB.Remove(*(nsHostKey *)head);

static_cast?

::: netwerk/dns/nsHostResolver.cpp:1328
(Diff revision 1)
>              }
>          }
>  
>          // If there are no more callbacks, remove the hash table entry
>          if (recPtr && recPtr->mCallbacks.isEmpty()) {
> -            mDB.Remove(static_cast<nsHostKey *>(recPtr));
> +            mRecordDB.Remove(*(nsHostKey *)recPtr);

static_cast?
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review215244

Err, didn't mean to tamper with the review flag.
Attachment #8939108 - Flags: review?(jthemphill)
Attachment #8939107 - Flags: review?(honzab.moz)
Attachment #8939108 - Flags: review?(honzab.moz)
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review215246
Attachment #8939108 - Flags: review?(jthemphill)
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review215488

::: netwerk/dns/nsHostResolver.cpp:534
(Diff revision 1)
>          PRCList *node = mEvictionQ.next;
>          while (node != &mEvictionQ) {
>              nsHostRecord *rec = static_cast<nsHostRecord *>(node);
>              node = node->next;
>              PR_REMOVE_AND_INIT_LINK(rec);
> -            mDB.Remove(static_cast<nsHostKey *>(rec));
> +            mRecordDB.Remove(*(nsHostKey *) rec);

yep, why not s_c ?

::: netwerk/dns/nsHostResolver.cpp:682
(Diff revision 1)
>  
>              nsHostKey key(nsCString(host), flags, af, nsCString(netInterface),
>                            originSuffix);
> -            auto he = static_cast<nsHostDBEnt*>(mDB.Add(&key, fallible));
> +            RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key);
> +            if (!entry) {
> +                entry = new nsHostRecord(key);

this is weird according https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119

this should not be needed.

::: netwerk/dns/nsHostResolver.cpp:690
(Diff revision 1)
> +            RefPtr<nsHostRecord> rec = entry;
>              // if the record is null, the hash table OOM'd.
> -            if (!he) {
> +            if (!rec) {
>                  LOG(("  Out of memory: no cache entry for host [%s%s%s].\n",
>                       LOG_HOST(host, netInterface)));
>                  rv = NS_ERROR_OUT_OF_MEMORY;

I think this is no longer needed according https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsTHashtable.h#449

::: netwerk/dns/nsHostResolver.cpp:1320
(Diff revision 1)
>          nsHostRecord* recPtr = nullptr;
>  
> -        for (RefPtr<nsResolveHostCallback> c : he->rec->mCallbacks) {
> +        for (RefPtr<nsResolveHostCallback> c : rec->mCallbacks) {
>              if (c->EqualsAsyncListener(aListener)) {
>                  c->remove();
> -                recPtr = he->rec;
> +                recPtr = rec.get();

you probably don't need .get()?
Priority: -- → P2
(In reply to Honza Bambas (:mayhemer) from comment #7)
> ::: netwerk/dns/nsHostResolver.cpp:682
> > +            RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key);
> > +            if (!entry) {
> > +                entry = new nsHostRecord(key);
> 
> this is weird according
> https://dxr.mozilla.org/mozilla-central/rev/
> 351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119
> 
> this should not be needed.

Technically true, but mRecordDB is a nsRefPtrHashtable, meaning that when not found, it allocates a refPtr and returns a reference to it. I still need to assign it.
I'll remove the `if (!rec)` branch, since the allocation is now infallible.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> Comment on attachment 8939107 [details]
> Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to
> release it
> 
> https://reviewboard.mozilla.org/r/209522/#review215486
> 
> won't we then leak the rec at
> https://dxr.mozilla.org/mozilla-central/rev/
> 351c75ab74c9a83db5c0662ba271b49479adb1f1/netwerk/dns/nsHostResolver.cpp#601 ?

This is blocking r.

(In reply to Valentin Gosu [:valentin] (PTO until Jan 8) from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > ::: netwerk/dns/nsHostResolver.cpp:682
> > > +            RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key);
> > > +            if (!entry) {
> > > +                entry = new nsHostRecord(key);
> > 
> > this is weird according
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119
> > 
> > this should not be needed.
> 
> Technically true, but mRecordDB is a nsRefPtrHashtable, meaning that when
> not found, it allocates a refPtr and returns a reference to it. I still need
> to assign it.
> I'll remove the `if (!rec)` branch, since the allocation is now infallible.

I'd rather check the final patch once again.  thanks.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review217208
Attachment #8939108 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #11)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > Comment on attachment 8939107 [details]
> > Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to
> > release it
> > 
> > https://reviewboard.mozilla.org/r/209522/#review215486
> > 
> > won't we then leak the rec at
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 351c75ab74c9a83db5c0662ba271b49479adb1f1/netwerk/dns/nsHostResolver.cpp#601 ?
> 
> This is blocking r.

I updated the patch to deal with the leak.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8939107 [details]
Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to release it

https://reviewboard.mozilla.org/r/209522/#review217556

thanks!
Attachment #8939107 - Flags: review?(honzab.moz) → review+
Comment on attachment 8939108 [details]
Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable

https://reviewboard.mozilla.org/r/209524/#review217578

in one way rb is a mess.  I missed you have updated the patch in the meantime.

thanks.
Attachment #8939108 - Flags: review+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aaa8ca204b8b
Use a refPtr for rec so that CompleteLookup doesn't need to release it r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/d739ece3bea2
Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/aaa8ca204b8b
https://hg.mozilla.org/mozilla-central/rev/d739ece3bea2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: