1660006 - Check GC works with 16KB page sizes on Apple arm64 hardware

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Currently all platforms we regularly test on have 4KB page size, but that will change with the new arm64 based hardware from Apple which will have 16KB page size. We need to check this doesn't break anything.

For example, I'm not sure we decommit pages where arena size is not the same as page size.

Comment 1

[Jon Coppeard (:jonco)]

Not much information but could be useful: https://developer.apple.com/documentation/apple_silicon/addressing_architectural_differences_in_your_macos_code

Comment 2

[Jon Coppeard (:jonco)]

It turns out we test on ARM64 hardware, so I think we can say GC works at least:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&group_state=expanded&searchStr=pixel%2Caarch64%2Cjit

Comment 3

[Jon Coppeard (:jonco)]

I can see a few possibilities for this:

1. Do nothing. Decommit won't work leading to much greater memory use.
2. Decommit where there are appropriately sized unused regions. This is reasonably simple to implement, and would give better memory use than 1.
3. Use a 16KB arena size on this platform. This is simple to implement and would give better memory use than 1.

I don't know which approach will have the best outcome in terms of memory use. However we can simulate the larger page size restriction with a build time configuration option and test different approaches without needing the actual hardware.

Comment 4

[Jon Coppeard (:jonco)]

Yoshi, is this something you could take a look at when you have a moment?

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 1: Config arena size to 16KB.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 2: add --enable-js-gc-arena-size-16kb in macosx64-aarch64.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

This week there's a try push caused some alert on perfherder
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&newProject=try&newRevision=9cde07932cb24d5e8f13528319211c2e99400734&framework=4&selectedTimeRange=172800

I tried a few other try pushes, those seems fine
arena-16KB:
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b4cd29abb74ae321be6176130fcd2130c7179d6d&newProject=try&newRevision=c8a6d1b97609693bec0b94def31f985ebe1a59ca+&framework=4

arena-16KB && page-size-16KB
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b4cd29abb74ae321be6176130fcd2130c7179d6d&newProject=try&newRevision=22879a4ca7bdcfb0e48dfd0fa8e98c761694cf70&framework=4

Comment 8

[Jon Coppeard (:jonco)]

Firstly these need to be run on MacOS to be as close as possible to the hardware we're targeting.

Then we need to do two comparisons:

1. Base vs 16KB page size

This should give us an idea of how much worse memory usage is on the new hardware if we do nothing. I would expect to see reasonably large regressions.

2.) 16KB page size vs 16KB page size and 16KB arena size

This should give us an idea of how much of an improvement using 16KB arenas is. It should show reduced memory usage, but probably not back to the initial levels.

You will need to use perfherder to retrigger the benchmarks 5 times to get meaningful 'confidence' results. If you hover over a value in the 'total runs' column a button will appear to let you do this.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

1. Base vs 16KB page size
   https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=0f5e4a3c6f0a6ab0fcd77c46cedc819e2bd0b3f8&newProject=try&newRevision=eac6d63cb58c0d3aabfda3a5b671c6e4ea58bec5&framework=4

2. 16KB page size vs (16KB page size + 16KB arena size)
   https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=eac6d63cb58c0d3aabfda3a5b671c6e4ea58bec5&newProject=try&newRevision=9fc60e40e5ba9fdecad9aaedb4240013b547f8ee&framework=4

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Perhherder says with 16KB page size + 16KB arena size, I got 20% regression in [Base JS After tabs open [+30s, forced GC] opt] compared with 16KB page size

https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=9fc60e40e5ba9fdecad9aaedb4240013b547f8ee&originalSignature=2203749&newSignature=2203749&framework=4&originalRevision=eac6d63cb58c0d3aabfda3a5b671c6e4ea58bec5

However I check the memory report, although on most pages in tp6 have decommitted size increase, but the explicit allocations are slightly smaller.
And in total I only find 1% memory increased

https://docs.google.com/spreadsheets/d/1duYIFImq7RGNpqf8-qjr_5Q6ikTryiWlEZBzDkxTOHc/edit?usp=sharing

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Previously in comment 10 I checked awsy, sy-tp6 report, there seems no significant change.

However after checking with other tests: ab, sy
https://treeherder.mozilla.org/jobs?repo=try&revision=9fc60e40e5ba9fdecad9aaedb4240013b547f8ee&selectedTaskRun=dcIkMRkaSGSwL2d3qoSp4w.0
https://treeherder.mozilla.org/jobs?repo=try&revision=9fc60e40e5ba9fdecad9aaedb4240013b547f8ee&selectedTaskRun=bYIFhTR6Raa1thej8XH4HA.0

I notice that unused-gc-things increases a lot.

I add another workaround to not to report gc-unused-things, and the memory regression goes awsy.

16KB Page Size + don't report unused-gc-things vs 16KB Page Size + 16KB Arena Size + don't report unused-gc-things
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=5f454c5b2a7af6580bd0a95ff3ec261db6daf036&newProject=try&newRevision=70c084f45bb0aa2f84d1b3fc5d4d27f713fa28ff&framework=4

Here's the perfherder compared to m-c
16KB Page Size + don't report unused-gc-things vs mozilla-central
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=ccc9365198a56d2b0bbcf7af68ed6c6a220c3331&newProject=try&newRevision=70c084f45bb0aa2f84d1b3fc5d4d27f713fa28ff&framework=4

and from unused/gc-things, objects, strings and shapes are the top 3 usages.
And the reason is for some arenas having small number of cells, its unused-gc-things will become larger because of larger ArenaSize.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - (For test) Config page size to 16KB

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - (For test awsy in perfherder) enable 16KB page size and 16KB arena size in linux64/macosx64 common-opt

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

calling DumpArenaInfo()
Original: https://pastebin.com/PXEGUvNP
ArenaSize 16KB: https://pastebin.com/GR2cwMZ2

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Below is the result of using 4KB Arena size on 16KB page size, and compare it to m-c

16KB page size vs 16KB page size + decommit
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=80f798495c4bdcf7fe76d662483cd5c386e3c2f5&newProject=try&newRevision=478753a7b40a27608465fe4e952e9847f2506948&framework=4

m-c vs 16KB page size + decommit
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=fc6258a70cb4192272b910170ec9f86774787792&newProject=try&newRevision=478753a7b40a27608465fe4e952e9847f2506948&framework=4

Base Content JS opt has 7.11% regression, the regression is from unused-arena got larger.

the memory report from 16KB page size + decommit
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/bwTFq4gyQHeqVQvfJ4D5yQ/runs/0/artifacts/public/test_info//memory-report-TabsOpenForceGC-0.json.gz

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - decommitFreeArenas

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi jonco, https://phabricator.services.mozilla.com/D104736 is the WIP for decommitFreeArenas

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

16KB page size vs 16KB page size + decommit

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=80f798495c4bdcf7fe76d662483cd5c386e3c2f5&newProject=try&newRevision=a80af2f51ad9b949293ca9c7314b3ccc199a5b93&framework=4

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - DecommitEnabled takes a length parameter.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - return false if !DecommitEnabled

Don't set decommittedArenas[i] to true if MarkPagesUnusedSoft returns
false.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonco
When the page size is 16KB, we didn't decommit, but we still set decommittedArenas to true.
https://searchfox.org/mozilla-central/rev/a23e65c5d69a821f61d14c8ec1f69a120e3f77d1/js/src/gc/GC.cpp#845
https://searchfox.org/mozilla-central/rev/5120ec68572d946bd15101cf2ee2eaf4a210724f/js/src/gc/Memory.cpp#774

I am wondering if we should NOT set decommittedArenas to true when we actually didn't decommit?
for example should we have the 2 patches below ?
https://phabricator.services.mozilla.com/D105347
https://phabricator.services.mozilla.com/D105348

Then the patch for decommitFreeArenas should see improvements.

Comment 22

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #21)
Good catch! This doesn't affect correctness but probably means the reported memory size is wrong. That would also explain why your patch didn't improve things as much as expected.

Can you file a bug for this?

The way to fix this is to check first whether we can decommit, and then don't attempt it (for now, and then your patch in this bug would fix this). Ideally we would just assert that the length was aligned to page size in the memory functions, rather than checking DecommitEnabled().

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #21)

https://phabricator.services.mozilla.com/D105347
https://phabricator.services.mozilla.com/D105348

(Page size 16KB + these two patches ) vs (Page size 16KB + these two patches + decommit patch)
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=05d6ff4b687d574c178d4270ed58de10daf1610f&newProject=try&newRevision=39a259fcee07439c5ce8e8f34e0846bbe5251130&framework=4

The memory footprint is much better.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - (WIP) decommit page.

WIP

Add a numPagesFreeCommitted in TenuredChunkInfo.

addArenaToFreeList --> check if there's a free page, if so, numPagesFreeCommitted++

decommitFreeArenas -> check if the chunk's numPagesFreeCommitted > 0, if
so, calling decommitOneFreePage, which in turns numPagesFreeCommitted--

When allocating an arena, check if the arena coming from a free page, if
so also doing numPagesFreeCommitted--

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 4: Update DecommitEnabled.

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Jonco, the patch already passes the try test,
with perfherder result https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=44150ddbd2977a583db89d5c768c94d21d383941&newProject=try&newRevision=c50c71f754e1ebe3d0635bd461867f70cb83fac4&framework=4
(compared to 16KB page)

Can you take a look at it?
Thanks

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 1: Add PageSize, ArenasPerPage, and PagesPerChunk.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 2: Rename decommittedPages and update commit behavior.

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 3: Update decommitFreeArenas.

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 4: Update numArenasFree before/after decommit system call.

Comment 31

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1660006 - Part 5: revise TenuredChunk::decommitFreeArenasWithoutUnlocking.

Comment 32

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(16KB page size) vs (16KB page size+ these patches)
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=321fd0a1cdfbee7feec0190c2f4736f55643bfcb&newProject=try&newRevision=9c8acf4714fe7f335d6cdace4f750f4062ab2429&framework=4
3%~23% improvement

m-c (4KB page size) vs these patches
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=05468a0dd9664c6a565835e5b0d43510b7809e6a&newProject=try&newRevision=061d05901c4cc90a5f9a6f6dd0cd3b5ada611a3a&framework=4
shows that it doesn't have regression on awsy/raptor

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7680fff70137
Part 1: Add PageSize, ArenasPerPage, and PagesPerChunk. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7bc19b9ba858
Part 2: Rename decommittedPages and update commit behavior. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8553a9cf2027
Part 3: Update decommitFreeArenas. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8fc5eb4145f6
Part 4: Update DecommitEnabled. r=jonco

Comment 34

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7680fff70137
https://hg.mozilla.org/mozilla-central/rev/7bc19b9ba858
https://hg.mozilla.org/mozilla-central/rev/8553a9cf2027
https://hg.mozilla.org/mozilla-central/rev/8fc5eb4145f6