Closed Bug 1227914 Opened 4 years ago Closed 4 years ago

Limit memory usage of TraceLogger ContinuousSpace

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Currently when there is not enough space in the ContinuousSpace, we will just double the about the memory used. Upon fail, we write/drop the data and reuse the memory we already had. Now it might be good to have some upper-limit that the ContinuousSpace can use. Just for safety.
Blocks: 1226961
Attached patch PatchSplinter Review
Put an upperbound on how big ContinuousSpace can get. Went for 200MB. Just to make sure we don't take all memory.
Assignee: nobody → hv1989
Attachment #8691922 - Flags: review?(benj)
Comment on attachment 8691922 [details] [diff] [review]
Patch

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

r=me if you're fine with the change below, which makes things easier to understand imo.

::: js/src/vm/TraceLoggingTypes.h
@@ +204,5 @@
> +        if (nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT) {
> +            if ((size_ + count) * sizeof(T) > CONTINUOUSSPACE_LIMIT)
> +                return false;
> +            nCapacity = size_ + count;
> +        }

It took me some time to realize why this is correct. Instead, can you do:

uint32_t nCapacity = capacity_ * 2;
if (size_ + count > nCapacity || nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT)
    nCapacity = size_ + count;
if (nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT)
    return false;
Attachment #8691922 - Flags: review?(benj) → review+
Or even

uint32_t nCapacity = capacity_ * 2;
if (size_ + count > nCapacity || nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT) {
    nCapacity = size_ + count;
    if (nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT)
        return false;
}
https://hg.mozilla.org/mozilla-central/rev/541a97a551cb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.