Closed
Bug 1091986
Opened 10 years ago
Closed 10 years ago
Change nsStreamLoader::mData to a mozilla::Vector.
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
1.08 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
6.58 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
nsStreamLoader has a buffer which gets grown in the smallest possible steps.
This means we sometimes see bad growth sequences like this:
> net: 9727 -> 10315
> net: 10315 -> 11299
> net: 11299 -> 14452
> net: 14452 -> 15438
> net: 15438 -> 18591
> net: 18591 -> 18782
> net: 18782 -> 21935
> net: 21935 -> 23496
> net: 23496 -> 26649
> net: 26649 -> 28395
A doubling growth strategy is typically used for growable buffers.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
Vector.h has several assertions involving CapacityHasExcessSpace(). These basically check that the heap storage is the size we expect based on our doubling algorithm. However, if you use initCapacity(), you can end up with a heap buffer that's not a standard size, and you can then trigger this assertion. So it should just be removed. The other similar assertions remain, because they are on paths that should only be reached once doubling has taken place.
Attachment #8514747 -
Flags: review?(luke)
![]() |
Assignee | |
Comment 2•10 years ago
|
||
This patch converts nsStreamLoader::mData to a mozilla::Vector, which uses a doubling growth strategy. (Note that the initial size of mData will be exactly that requested; the doubling only kicks when a buffer is resized.) Loading Gmail, TechCrunch and CNN, this reduces the heap churn from 7.5 to 7.2 MiB and reduces the number of reallocations from 126 to 82. It will also help with heap fragmentation by avoiding unusual allocation sizes; see bug 1048044 comment 35 for a similar case. It also makes the code more concise and less error-prone, by avoiding the manual buffer management. And because mozilla::Vector uses size_t for its length, we can avoid some checking code involving UINT32_MAX.
Attachment #8514748 -
Flags: review?(mcmanus)
![]() |
||
Updated•10 years ago
|
Attachment #8514747 -
Flags: review?(luke) → review+
Comment 3•10 years ago
|
||
Comment on attachment 8514748 [details] [diff] [review] (part 2) - Change nsStreamLoader::mData to a mozilla::Vector Review of attachment 8514748 [details] [diff] [review]: ----------------------------------------------------------------- awesome. thanks.
Attachment #8514748 -
Flags: review?(mcmanus) → review+
![]() |
Assignee | |
Comment 4•10 years ago
|
||
I had to tweak the extractRawBuffer part slightly to get past some test failures. The new version looks good on try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=74081468c6f8
![]() |
Assignee | |
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24680cc584c https://hg.mozilla.org/integration/mozilla-inbound/rev/4be4cf0afc29
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b24680cc584c https://hg.mozilla.org/mozilla-central/rev/4be4cf0afc29
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
![]() |
Assignee | |
Updated•10 years ago
|
Depends on: cumulative-heap-profiling
You need to log in
before you can comment on or make changes to this bug.
Description
•