Closed Bug 1857841 Opened 10 months ago Closed 4 months ago

Try to reduce VirtualAlloc calls in mozjemalloc

Categories

(Core :: Memory Allocator, task)

task

Tracking

()

RESOLVED FIXED
126 Branch
Tracking Status
firefox126 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 2 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Jeff Muizelaar found that mozjemalloc's allocation patterns can cause us to make very frequent calls to VirtualAlloc, at least more frequent that Chrome does.

He wrote:
pbone: another related thing that I saw today: During a run of 10 iterations of TodoMVC-> jquery we're doing 20x the number VirtualAllocs/VirtualFrees that Chrome is
[they're comming from] splitRun [see] https://share.firefox.dev/45scxBe

Off the top of my head, the only way to reduce the number of these VirtualAllocs would be to have something akin to the double purge we have on mac, and in any case, that will have an impact on RSS, since these VirtualAllocs are there to reduce memory usage (they decommit/recommit memory).

Depends on: 1862378
Blocks: 1867197
Assignee: nobody → pbone
Status: NEW → ASSIGNED

Depends on D195511

This breaks the old invariant that there are no committed pages that aren't
either allocated or dirty. There is now also new code in DeallocChunk that
counts the number of "fresh" committed pages and adjusts mStats.committed.

Depends on D195513

Actually the difference is in how the page cache is measured as a kind of explicit memory. But these pages aren't paged in and you can see that there's no difference to resident memory. So I think the memory usage difference is actually negligible.

Attachment #9366991 - Attachment description: WIP: Bug 1857841 - pt 2. InitChunk refactoring → WIP: Bug 1857841 - pt 3. InitChunk refactoring
Attachment #9366992 - Attachment description: WIP: Bug 1857841 - pt 3. Don't decommit then recommit pages in InitChunk → WIP: Bug 1857841 - pt 4. Don't decommit then recommit pages in InitChunk
Attachment #9366993 - Attachment description: WIP: Bug 1857841 - pt 4. Commit more pages than necessary to amoritse costs → WIP: Bug 1857841 - pt 5. Commit more pages than necessary to amoritse costs
Attachment #9366994 - Attachment description: WIP: Bug 1857841 - pt 5. Also amortise VirtualAlloc in SplitRun() → WIP: Bug 1857841 - pt 6. Also amortise VirtualAlloc in SplitRun()

The decommit logic is now only used when MALLOC_DECOMMIT is defined. Now
the CHUNK_MAP_MADVISED bit is cleared when the page is initialised avoiding
leaving the chunk map in an inconsistent state if pages_commit failes and
simplifying some logic.

These changes will make the next patch simpler.

This memory was previously measured as part of committed memory. However
since it probably has no physical page allocated it's more correct to count
it as clean. This has the side effect of now including it in the page cache
and therefore decisions like when to purge memory will change.

Attachment #9366990 - Attachment description: WIP: Bug 1857841 - pt 1. Modernise variable declarations in SplitRun → Bug 1857841 - pt 1. Modernise variable declarations in SplitRun r=glandium
Attachment #9377860 - Attachment description: WIP: Bug 1857841 - pt 2. Remove obsolete comment → Bug 1857841 - pt 2. Remove obsolete comment r=glandium
Attachment #9366991 - Attachment description: WIP: Bug 1857841 - pt 3. InitChunk refactoring → Bug 1857841 - pt 3. InitChunk refactoring r=glandium
Attachment #9366992 - Attachment description: WIP: Bug 1857841 - pt 4. Don't decommit then recommit pages in InitChunk → Bug 1857841 - pt 4. Don't decommit then recommit pages in InitChunk r=glandium
Attachment #9366993 - Attachment description: WIP: Bug 1857841 - pt 5. Commit more pages than necessary to amoritse costs → Bug 1857841 - pt 5. Commit more pages than necessary to amoritse costs r=glandium
Attachment #9366994 - Attachment description: WIP: Bug 1857841 - pt 6. Also amortise VirtualAlloc in SplitRun() → Bug 1857841 - pt 6. Also amortise VirtualAlloc in SplitRun() r=glandium
Attachment #9377861 - Attachment description: WIP: Bug 1857841 - pt 7. Simplify decommit logic in SplitRun() → Bug 1857841 - pt 7. Simplify decommit logic in SplitRun() r=glandium
Attachment #9377862 - Attachment description: WIP: Bug 1857841 - pt 8. Count clean memory within mozjemalloc → Bug 1857841 - pt 8. Count clean memory within mozjemalloc r=glandium
Blocks: 1878806
Attachment #9366992 - Attachment description: Bug 1857841 - pt 4. Don't decommit then recommit pages in InitChunk r=glandium → Bug 1857841 - pt 3. Don't decommit then recommit pages in InitChunk r=glandium
Attachment #9366993 - Attachment description: Bug 1857841 - pt 5. Commit more pages than necessary to amoritse costs r=glandium → Bug 1857841 - pt 4. Commit more pages than necessary to amortise costs r=glandium
Attachment #9366994 - Attachment description: Bug 1857841 - pt 6. Also amortise VirtualAlloc in SplitRun() r=glandium → Bug 1857841 - pt 5. Also amortise VirtualAlloc in SplitRun() r=glandium
Attachment #9377861 - Attachment description: Bug 1857841 - pt 7. Simplify decommit logic in SplitRun() r=glandium → Bug 1857841 - pt 6. Simplify decommit logic in SplitRun() r=glandium
Attachment #9377862 - Attachment description: Bug 1857841 - pt 8. Count clean memory within mozjemalloc r=glandium → Bug 1857841 - pt 7. Count clean memory within mozjemalloc r=glandium
Attachment #9366991 - Attachment is obsolete: true

CHUNK_MAP_MADVISED is also used for never-touched pages. Rename it
CHUNK_MAP_CLEAN since it's the clean page cache.

Attachment #9381892 - Attachment is obsolete: true

Also re-write the descriptions of the page kinds.

Using CHUNK_MAP_FRESH for double purged pages fits better than overloading
the CHUNK_MAP_DECOMMITTED bit with two different semantics.

Attachment #9366992 - Attachment description: Bug 1857841 - pt 3. Don't decommit then recommit pages in InitChunk r=glandium → Bug 1857841 - pt 5. Don't decommit then recommit pages in InitChunk r=glandium
Attachment #9366993 - Attachment description: Bug 1857841 - pt 4. Commit more pages than necessary to amortise costs r=glandium → Bug 1857841 - pt 7. Commit more pages than necessary to amortise costs r=glandium
Attachment #9366994 - Attachment description: Bug 1857841 - pt 5. Also amortise VirtualAlloc in SplitRun() r=glandium → Bug 1857841 - pt 8. Also amortise VirtualAlloc in SplitRun() r=glandium
Attachment #9377862 - Attachment description: Bug 1857841 - pt 7. Count clean memory within mozjemalloc r=glandium → Bug 1857841 - pt 9. Count fresh and madvised memory separately r=glandium
Blocks: 1883823
Attachment #9366992 - Attachment description: Bug 1857841 - pt 5. Don't decommit then recommit pages in InitChunk r=glandium → Bug 1857841 - pt 4. Don't decommit then recommit pages in InitChunk r=glandium
Attachment #9377861 - Attachment description: Bug 1857841 - pt 6. Simplify decommit logic in SplitRun() r=glandium → Bug 1857841 - pt 5. Simplify decommit logic in SplitRun() r=glandium
Attachment #9366993 - Attachment description: Bug 1857841 - pt 7. Commit more pages than necessary to amortise costs r=glandium → Bug 1857841 - pt 6. Commit more pages than necessary to amortise costs r=glandium
Attachment #9366994 - Attachment description: Bug 1857841 - pt 8. Also amortise VirtualAlloc in SplitRun() r=glandium → Bug 1857841 - pt 7. Also amortise VirtualAlloc in SplitRun() r=glandium
Attachment #9377862 - Attachment description: Bug 1857841 - pt 9. Count fresh and madvised memory separately r=glandium → Bug 1857841 - pt 8. Count fresh and madvised memory separately r=glandium
Attachment #9389308 - Attachment description: Bug 1857841 - pt 4. Use CHUNK_MAP_FRESH for double-purged pages r=glandium → Bug 1857841 - pt 8. Use CHUNK_MAP_FRESH for double-purged pages r=glandium
Attachment #9377862 - Attachment description: Bug 1857841 - pt 8. Count fresh and madvised memory separately r=glandium → Bug 1857841 - pt 9. Count fresh and madvised memory separately r=glandium

I'm waiting for the code freeze to end before landing this.

Blocks: 1885466

Here's the latest performance comparison:

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=cd81556b73b839957e9de7370c2978718e26fbcf&newProject=try&newRevision=7afe1cfe228d6c3cc43b92778425d0edb4d9b422&framework=13&page=1

There are tests in both "browsertime" and "awsy". In the awsy tests the big difference in explicit memory is a change in how we measure memory - not a real memory difference, for that please look at "resident".

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a09b357080f
pt 1. Modernise variable declarations in SplitRun r=glandium
https://hg.mozilla.org/integration/autoland/rev/83e751e19af4
pt 2. Remove obsolete comment r=glandium
https://hg.mozilla.org/integration/autoland/rev/fe61be13535d
pt 3. Add a new page kind named "fresh" r=glandium
https://hg.mozilla.org/integration/autoland/rev/1a70135ddbda
pt 4. Don't decommit then recommit pages in InitChunk r=glandium
https://hg.mozilla.org/integration/autoland/rev/afe7c24ca50e
pt 5. Simplify decommit logic in SplitRun() r=glandium
https://hg.mozilla.org/integration/autoland/rev/52f8506f3bfb
pt 6. Commit more pages than necessary to amortise costs r=glandium
https://hg.mozilla.org/integration/autoland/rev/f335f70a1c14
pt 7. Also amortise VirtualAlloc in SplitRun() r=glandium
https://hg.mozilla.org/integration/autoland/rev/82252f397be4
pt 9. Count fresh and madvised memory separately r=glandium
https://hg.mozilla.org/integration/autoland/rev/86d16de09fc9
pt 8. Use CHUNK_MAP_FRESH for double-purged pages r=glandium
https://hg.mozilla.org/integration/autoland/rev/37720af4970f
pt 10. Tighten an assertion r=glandium
Regressions: 1888326
See Also: → 1898646
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: