Closed Bug 1555243 Opened 1 year ago Closed 1 year ago

Document decommitAllArenasWithoutUnlocking's complexity

Categories

(Core :: JavaScript: GC, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: pbone, Assigned: pbone)

Details

Attachments

(1 file)

decommitAllArenasWithoutUnlocking can be witten without a loop, this will reduce its potential 255 system calls with a single one.

(In reply to Paul Bone [:pbone] from comment #0)
This only decommits arenas that aren't in use. I don't see how you can do that with a single system call.

Hrm, we can do that with madvise on Linux / macos etc. But I don't know about windows. ehoogeveen, do you know?

Flags: needinfo?(emanuel.hoogeveen)
Summary: Reimplement :decommitAllArenasWithoutUnlocking without a loop → Reimplement decommitAllArenasWithoutUnlocking without a loop

The patch is simple, we could write it and find out ;-)

(In reply to Paul Bone [:pbone] from comment #3)
I'm still not sure how this would work. Also, this method is only called on OOM, so there isn't much point optimising it.

The Windows equivalent of madvise with MADV_DONTNEED or MADV_FREE would be VirtualAlloc with MEM_RESET, which we currently use as a soft decommit on Windows. The only additional restriction on Windows is that VirtualAlloc and VirtualFree calls can't span multiple mappings - but I assume we'd only be interested in doing this within chunks anyway (and on 64-bit platforms our chunks are very unlikely to be anywhere near each other).

Neither of these operations give any assurances that the underlying data will be preserved though, so to batch decommit we'd need some way to find contiguous unused arenas. jemalloc does something like this using a red-black tree, but that seems like overkill. If we're doing a system call for each arena right now then just doing a linear scan would probably be an improvement.

Flags: needinfo?(emanuel.hoogeveen)

With some arenas allocated the patch is more complex..(In reply to Jon Coppeard (:jonco) from comment #4)

(In reply to Paul Bone [:pbone] from comment #3)
I'm still not sure how this would work. Also, this method is only called on OOM, so there isn't much point optimising it.

Only if you wanted to use it as you suggested in your review for Bug 1499570.

Instead of making it more complex I'll add a comment to explain that it performs a system call for each arena.

Assignee: nobody → pbone
Status: NEW → ASSIGNED
Summary: Reimplement decommitAllArenasWithoutUnlocking without a loop → Document decommitAllArenasWithoutUnlocking's complexity
Attachment #9068584 - Attachment description: Bug 1555243 - Document decommitAllArenasWithoutUnlocking's complexity → Bug 1555243 - Document decommitAllArenasWithoutUnlocking's complexity r=jonco

(In reply to Paul Bone [:pbone] from comment #6)

Only if you wanted to use it as you suggested in your review for Bug 1499570.

I suggested using Chunk::decommitAllArenas, not Chunk::decommitAllArenasWithoutUnlocking.

(In reply to Jon Coppeard (:jonco) from comment #9)

(In reply to Paul Bone [:pbone] from comment #6)

Only if you wanted to use it as you suggested in your review for Bug 1499570.

I suggested using Chunk::decommitAllArenas, not Chunk::decommitAllArenasWithoutUnlocking.

Sorry, my bad. I discovered that anyway while reading code yesterday and submitted Bug 1555550

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43a872e4cee6
Document decommitAllArenasWithoutUnlocking's complexity r=jonco
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.