Change nsStreamLoader::mData to a mozilla::Vector.

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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

5 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

5 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

5 years ago
Attachment #8514747 - Flags: review?(luke) → review+
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

5 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
https://hg.mozilla.org/mozilla-central/rev/b24680cc584c
https://hg.mozilla.org/mozilla-central/rev/4be4cf0afc29
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

5 years ago
You need to log in before you can comment on or make changes to this bug.