nsIDNSService::Resolve does not use the dns cache

VERIFIED FIXED in mozilla0.9.9

Status

()

P1
major
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: bbaetz, Assigned: gordon)

Tracking

({perf})

Trunk
mozilla0.9.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
The synchronous dns resolve method doesn't use the DNS cache. This is bad
because this function is used often in the PAC code, which must do a synchronous
lookup. The same host is looked up more than once in lots of pac files for
functions like isInNet, and I think that everyone assumed that the dns cache
would get this.

(Although I am going to suggest that the pac code does a one-entry cache, like
the mozillaClassic code did, to avoid unneeded xpconnect wrapping.
(Reporter)

Updated

17 years ago
Blocks: 93924
(Assignee)

Comment 1

17 years ago
This seems doable in the 0.9.6 timeframe.
Target Milestone: --- → mozilla0.9.6
(Reporter)

Updated

17 years ago
Blocks: 97605
*** Bug 104819 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

17 years ago
Moving to necko performance milestone 0.9.7.
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Comment 4

17 years ago
Created attachment 60795 [details] [diff] [review]
attaching patch

Please review the attached patch and let me know if there are problems.

Comment 5

17 years ago
Created attachment 61235 [details] [diff] [review]
Protect table lookup with lock

Darin pointed out that the hash table lookup needs to be protected by the dns
lock.
Doing so.
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

17 years ago
Keywords: perf
(Assignee)

Updated

17 years ago
Priority: -- → P1
Whiteboard: patch

Updated

17 years ago
Keywords: nsbeta1+
(Assignee)

Comment 6

17 years ago
Created attachment 69532 [details] [diff] [review]
updated patch to protect access of lookup's buffer.  Removed unnecessary Addref & Release.
(Assignee)

Comment 7

17 years ago
Gagan, Darin, I just submitted a new patch for your [super-]review.  Is there a
good stress test for Resolve()?
(Assignee)

Comment 8

17 years ago
Gagan, Darin, please review.
(Assignee)

Comment 9

17 years ago
Created attachment 70212 [details] [diff] [review]
replaces inet_ntoa() whose thread-safety is somewhat dubious.
Attachment #60795 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #61235 - Attachment is obsolete: true
Attachment #69532 - Attachment is obsolete: true
(Assignee)

Comment 10

17 years ago
Comment on attachment 70212 [details] [diff] [review]
replaces inet_ntoa() whose thread-safety is somewhat dubious.

sr=darin
Attachment #70212 - Flags: superreview+

Comment 11

17 years ago
Comment on attachment 70212 [details] [diff] [review]
replaces inet_ntoa() whose thread-safety is somewhat dubious.

r=gagan
Attachment #70212 - Flags: review+
(Assignee)

Comment 12

17 years ago
Patch checked in.  Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 13

17 years ago
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.