Closed Bug 1348041 Opened 7 years ago Closed 7 years ago

Change default of network.http.max-urgent-start-excessive-connections-per-host to 3

Categories

(Core :: Networking: HTTP, enhancement)

Product:
Core
Component:
Networking: HTTP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mayhemer, Assigned: amchung)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 4 obsolete files)

implementation
9.99 KB, patch
amchung
: review+
Details | Diff | Splinter Review 
To mitigate bug 1345845 let's lower the number of excessive urgent start connections.

Amy, can you do it please?  I missed during the last review that you didn't change the value to 4 as I wanted.  And when we are here, I think 3 connections will do.  Thanks.
Attached patch implementation (obsolete) — — Splinter Review 
Hi Honza,
Sorry I didn't change the default of max-urgent-start-excessive-connections-per-host, and I finished to modify it on the patch:
1. Modified the default of network.http.max-urgent-start-excessive-connections-per-host to 3 on all.js
2. Modified the defalut of mMaxUrgntStartQ to 3 on constructor nsHttpHandler().

Would you help me to review my patch?
Thanks!
Attachment #8849047 - Flags: review?(honzab.moz)
Amy, I have one more thing.  The members and some argument should also be renamed from MaxUrgentQ to something more reflecting the new meaning of the preference.  Like MaxUrgentExcessiveConns or something.  Would you please do it as part of this patch too?
Flags: needinfo?(amchung)
Attached patch implementation (obsolete) — — Splinter Review 
Hi Honza,
I have replaced all MaxUrgent to MaxUrgentExcessiveConns.
Would you help me to review my patch?

Thanks!
Attachment #8849047 - Attachment is obsolete: true
Attachment #8849047 - Flags: review?(honzab.moz)
Flags: needinfo?(amchung)
Attachment #8849434 - Flags: review?(honzab.moz)
Comment on attachment 8849434 [details] [diff] [review]
implementation

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

Thank you!
Attachment #8849434 - Flags: review?(honzab.moz) → review+
Amy, any reason not to land this now?
Flags: needinfo?(amchung)
Attached patch implementation (obsolete) — — Splinter Review 
Hi Honza,
Sorry for I was waiting for the testing result of try server.
Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f60693a7da7134b87d4f425989ba6819404f56b&selectedJob=86642360
Attachment #8849434 - Attachment is obsolete: true
Flags: needinfo?(amchung)
Attachment #8851677 - Flags: review+
Keywords: checkin-needed
Aha, no problem :)
Needs rebasing.
Flags: needinfo?(amchung)
Keywords: checkin-needed
Keywords: checkin-needed
patching file netwerk/protocol/http/nsHttpConnectionMgr.cpp
Hunk #3 FAILED at 1031
1 out of 4 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsHttpConnectionMgr.cpp.rej

I'm guessing it was changes on inbound that only recently got merged around?
Keywords: checkin-needed
Attached patch implementation — — Splinter Review 
Already rebased the latest changes on nsHttpConnectionMgr.cpp.
Attachment #8852017 - Attachment is obsolete: true
Attachment #8852116 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a8765f0474
Change default value of network.http.max-urgent-start-excessive-connections-per-host to 3. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8a8765f0474
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: