MallocMS page accounting #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

qinsoon merged 15 commits into mmtk:master from qinsoon:malloc-ms-page-accounting

Nov 7, 2022

Member

qinsoon commented Oct 27, 2022 •

edited by k-sareen

Loading

This PR changes how our malloc mark sweep accounts for memory. We are currently accounting bytes used, which is wrong. This PR changes that to page-based accounting. This PR addresses the issue #649 for malloc mark sweep (further work is still needed for the issue for our malloc API).

by default, we do not use bulk XOR for malloc mark sweep (which is known to have issues with page accounting)
Allow different malloc page size for library malloc

k-sareen and others added 7 commits

October 27, 2022 10:25


          GC space overhead

0d032ba


          Allow sweep each object for side mark bit

1a6e19e


          Measure mimalloc 64K pages.

b867bc2


          Remove unneeded changes.

76fa950


          Minor clean up

65935da


          Fix style check

203becb


          Fix API check

0f73a12

qinsoon added the PR-testing label

Collaborator

k-sareen commented Oct 27, 2022 •

edited

Loading

I have a minor fix for a concurrency bug I noticed. Should I make the PR instead?

EDIT: My fix actually simplifies some of the PR as it removes the extra parameter from initialize_object_metadata. See here: k-sareen@ab2c758

Member Author

qinsoon commented Oct 27, 2022

I have a minor fix for a concurrency bug I noticed. Should I make the PR instead?

Sure. You could also commit to this PR if you would like to.

Collaborator

k-sareen commented Oct 27, 2022 •

edited

Loading

I have a minor fix for a concurrency bug I noticed. Should I make the PR instead?

Sure. You could also commit to this PR if you would like to.

Okay will do 👍

EDIT: Hmm @qinsoon I think the PR is from your personal mmtk-core fork so I don't know how I can push to it.


          Fix concurrency bug in accessing active page metadata

1bd15ad

Use a compare_exchange for accessing active page metadata instead of
load followed by a store

k-sareen changed the title ~~Malloc ms page accounting~~ MallocMS page accounting


          Fix build errors

a74ca18

Collaborator

k-sareen commented Oct 28, 2022

@qinsoon It's an OOM error for JikesRVM as we've changed how the memory accounting works. We should just be able to bump the heap size and it should pass (theoretically).

Member Author

qinsoon commented Oct 28, 2022

@qinsoon It's an OOM error for JikesRVM as we've changed how the memory accounting works. We should just be able to bump the heap size and it should pass (theoretically).

Right. I am doing that in mmtk/mmtk-jikesrvm#128.

Collaborator

k-sareen commented Oct 28, 2022

Ah right -- missed that PR. Thank you 👍

Member Author

qinsoon commented Oct 28, 2022

binding-refs
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-pr-689

qinsoon added PR-testing and removed PR-testing labels


          Minor clean up

710370a

qinsoon marked this pull request as ready for review

October 31, 2022 01:02

qinsoon mentioned this pull request

Update to MMTk core PR #689. Update mark sweep tests heap size. mmtk/mmtk-jikesrvm#128

Merged

qinsoon requested a review from wks

October 31, 2022 01:03

k-sareen reviewed

View reviewed changes

src/policy/mallocspace/global.rs Outdated Show resolved Hide resolved

wks reviewed

View reviewed changes

Collaborator

wks left a comment

I identified some performance issue. If an object is too large (more than one page), it will need to set multiple metadata bits, and the current implementation is inefficient. See in-line comments for possible solutions.

src/policy/mallocspace/global.rs Outdated

+                      // It is important to go to the end of the object, which may span a page boundary
+                      while page < start + size {
+                          if compare_exchange_set_page_mark(page) {
+                              self.active_pages.fetch_add(1, Ordering::SeqCst);

Collaborator

wks Oct 31, 2022

The cmpxchg and the fetch_add happen as two separate operations. One possible optimisation is working out locally how many page marks are set in this loop, and do one single fetch_all(n) after the loop.

Member Author

qinsoon Nov 1, 2022

Fixed. Now I am using a local variable to store the number of pages, and do one fetch_add in the end. I also did the same for unset_page_mark.

src/policy/mallocspace/global.rs Show resolved Hide resolved

src/policy/mallocspace/global.rs Outdated

+                      let mut page = chunk_start;
+                      while page < chunk_start + BYTES_IN_CHUNK {
+                          self.unset_page_mark(page);

Collaborator

wks Oct 31, 2022

Same here. You probably don't want to set one bit at a time.

Member Author

qinsoon Nov 1, 2022

I changed to unset_page_mark(start, size). It does something similar to the set_page_mark(), and use bulk zero in the end.

src/policy/mallocspace/global.rs Outdated Show resolved Hide resolved

qinsoon added 4 commits

November 1, 2022 11:05


          Optimize set/unset page mark, according to the reviews

a834d57


          Install mdbook using stable toolchain

70a17ed


          Avoid subtraction overflow

8b978b2


          Avoid subtraction overflow

39b87f7

wks approved these changes

View reviewed changes

Collaborator

wks left a comment

It looks good to me now. I don't see more problems.

Member Author

qinsoon commented Nov 1, 2022

The JikesRVM binding keeps failing. I will look at that tomorrow.

Collaborator

k-sareen commented Nov 2, 2022

The JikesRVM binding keeps failing. I will look at that tomorrow.

Yeah I'm not so sure of the bzero for the page metadata. I think it's correct but it's harder to reason about than the set/unset each bit. Since we're going through each page to update the local variable regardless, may as well change it from an is_page_marked() to compare_exchange_unset_page_mark() which might fix the issue with the JikesRVM binding as well.


          Unset each page mark rather than bulk zero. No need to use peek for

c473fbd

iterating objects.

Member Author

qinsoon commented Nov 3, 2022

The JikesRVM binding keeps failing. I will look at that tomorrow.

Yeah I'm not so sure of the bzero for the page metadata. I think it's correct but it's harder to reason about than the set/unset each bit. Since we're going through each page to update the local variable regardless, may as well change it from an is_page_marked() to compare_exchange_unset_page_mark() which might fix the issue with the JikesRVM binding as well.

That seems to be the problem, though I don't know why bulk zero does not work. I have reverted that part of the code.

qinsoon added PR-testing and removed PR-testing labels

Member Author

qinsoon commented Nov 7, 2022

After fixing the issue above, I am still seeing failures frequently. But I suspect it is the same problem as mmtk/mmtk-jikesrvm#108 (which is not reproducible on our dev machines). As this PR changes how we account memory for malloc mark sweep and we actually increase the min heap size for benchmarks, GCs are triggered more frequently and therefore we see failures more frequently. I changed the heap size we use for malloc mark sweep in JikesRVM to mitigate the issue.

qinsoon merged commit 83fa8ec into mmtk:master

qinsoon added a commit to mmtk/mmtk-jikesrvm that referenced this pull request


          Update to MMTk core PR #689. Update mark sweep tests heap size. (#128)

feb3400

This PR updates to mmtk-core mmtk/mmtk-core#689. With the changes in mmtk-core, mark sweep does page accounting, will require a larger heap size to run some benchmarks.

qinsoon mentioned this pull request

Memory consumption has gone up mmtk/mmtk-ruby#19

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels