Closed Bug 1388935 Opened 7 years ago Closed 7 years ago

Very large allocations shouldn't be recycled in their entirety

Categories

(Core :: Memory Allocator, defect)

Product:
Core
Component:
Memory Allocator
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(2 files)

Bug 1388935 - Inline chunk_dalloc_mmap into chunk_dealloc.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1388935 - Avoid overflowing the chunk recycling limit with very large chunks.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
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: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: