Closed Bug 1388935 Opened 4 years ago Closed 4 years ago

Very large allocations shouldn't be recycled in their entirety

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(2 files)

Chunk recycling will happily keep chunks larger than the recycle limit of 128MB, potentially limiting the amount of address space available to other things. That would be less of a problem if everything address space allocation was middle-manned by jemalloc, but that's not the case for e.g. js.

STR:
- Write a file with the following content:
1 1 jemalloc_stats()
1 1 malloc(2097152)=#1
1 1 free(#1)
1 1 jemalloc_stats()
1 1 malloc(2147483648)=#1
1 1 free(#1)
1 1 jemalloc_stats()
- Run `gdb $objdir/dist/bin/logalloc-replay`
- Add a breakpoint on jemalloc_stats
- Launch the program with the file created above as input (`run < /path/to/file`)
- Each time the breakpoint is hit, look at `Virtual memory size` in the output for `info proc stat`, and continue execution.

ER:
- The third value should not be much more than the first value + 128MB, but it actually is greater than first value + 2GB.
Comment on attachment 8895611 [details]
Bug 1388935 - Inline chunk_dalloc_mmap into chunk_dealloc.

https://reviewboard.mozilla.org/r/166836/#review171958
Attachment #8895611 - Flags: review?(n.nethercote) → review+
Comment on attachment 8895612 [details]
Bug 1388935 - Avoid overflowing the chunk recycling limit with very large chunks.

https://reviewboard.mozilla.org/r/166838/#review171960

::: memory/mozjemalloc/mozjemalloc.cpp:2193
(Diff revision 1)
> +			// Drop pages that would overflow the recycle limit
> +			if (to_recycle < size) {
> +				pages_trim(chunk, size, 0, to_recycle);
> +			}
> +			chunk_record(&chunks_szad_mmap, &chunks_ad_mmap, chunk, to_recycle, type);
> +		}

I found this hard to read, esp. due to the use of std::min(). Here's a version that I believe is equivalent, but I find easier to read:

> size_t recycle_remaining = recycle_limit - recycled_so_far;
> size_t to_recycle;
> if (size > recycle_remaining) {
>     to_recycle = recycle_remaining;
>     // Drop pages that would overflow the recycle limit.
>     pages_trim(chunk, size, 0, to_recycle);
> } else {
>     to_recycle = size;
> }   
> chunk_record(&chunks_szad_mmap, &chunks_ad_mmap, chunk, to_recycle, type);

What do you think?

BTW, now I'm wondering if `to_recycle` is the right value for the last argument of `pages_trim()`?
(In reply to Nicholas Nethercote [:njn] from comment #4)
> What do you think?

WFM

> BTW, now I'm wondering if `to_recycle` is the right value for the last
> argument of `pages_trim()`?

The function signature is very confusing indeed, but it's the right value. I actually started refactoring some of the chunk related code after bug 1360772, defining a class for chunks, containing their location and size, but never got around to finish that. With such refactor, pages_trim could become trim_left and trim_right on the class, and would become more palatable, but that would need something different for Windows.
Comment on attachment 8895612 [details]
Bug 1388935 - Avoid overflowing the chunk recycling limit with very large chunks.

https://reviewboard.mozilla.org/r/166838/#review172064
Attachment #8895612 - Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/dfb4a15b0c32
Inline chunk_dalloc_mmap into chunk_dealloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/208572c7abae
Avoid overflowing the chunk recycling limit with very large chunks. r=njn
Ha, I didn't move the return when changing per comment 4.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/fb3dc933a5b7
Inline chunk_dalloc_mmap into chunk_dealloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/5f48878bd935
Avoid overflowing the chunk recycling limit with very large chunks. r=njn
https://hg.mozilla.org/mozilla-central/rev/fb3dc933a5b7
https://hg.mozilla.org/mozilla-central/rev/5f48878bd935
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.