Allow stress test to trigger at byte-level granularity

[k-sareen]

This PR re-hauls the Stress GC trigger mechanism in MMTk so that we can trigger a GC at byte-level granularity as opposed to the page-level granularity we have right now. This mechanism will also be appropriated by the analysis tracer (such as sanity) in future PRs.

Note: I have only tested this for the openjdk binding. I have informally tested the mechanism on small test cases such as fop.

[qinsoon]

There seems to be an issue for running workflows from a fork. I will work on that today.

[k-sareen]

Using secrets in a pull request from a fork: https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request_target
https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

Most likely what we need

[qinsoon]

Using secrets in a pull request from a fork: https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request_target
https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

Most likely what we need

Thanks for the links. I have got a seemingly working fix here: #200. The only issue I noticed is that it cannot post perf results as comments (as GITHUB_TOKEN from a pull request does not have permission to post comments). We can still see the results as artefacts for the runs. Switching to use pull_request_target could solve this. I am not using it at the moment, mainly because it needs to be in the base branch and I cannot test it from a PR. I may later do another PR to use pull_request_target.

You can wait until #200 gets merged (should be tomorrow), and then merge with master. Hopefully things will work.

[qinsoon]

There are two issues for this PR.

1. There are race conditions and the implementation does not guarantee precise triggering per bytes. For example, we would like to see stress GC for every 8 bytes allocation, and we are allocating 8 bytes objects in two mutator threads. In this line

   mmtk-core/src/util/alloc/bumpallocator.rs

   Line 121 in 201d442

   && (base.allocation_count.load(Ordering::Relaxed) > base.options.stress_factor)

   both threads may see allocation_count == 0 and pass the check. In this case, they both allocate from their local buffer and no stress GC is triggered with 16 bytes allocated. To fix this, you probably would need to restructure the code a bit and use atomic operations to check and update the allocation count.
2. The implementation is very specific about bump allocator. For example, large object allocator does not trigger stress GC in this implementation. In my understanding, we should have some general code in allocator.rs which works for all allocators (e.g. check and add allocation bytes), and have some allocator-specific code that each allocator is required to implement (e.g. how to allocate when we are in slow path but do not need to acquire a new block).

[k-sareen]

Since we are calling all shots off on performance, we can address the first point by using sequential consistency. That'll force the two threads to synchronize in sense.

For the second, I'll refactor updating the allocation count semantics into alloc_slow_inline. On a similar note, is it expected for the PR to change other allocators such as large object etc?

[qinsoon]

For the second, I'll refactor updating the allocation count semantics into alloc_slow_inline. On a similar note, is it expected for the PR to change other allocators such as large object etc?

Yeah, I hope so. It is a good test to see if you have cleanly separated general code and allocator-specific code. Large object allocator does not have a fastpath, which means once you have separated the code, you should need minimal changes to large object allocator. If things are not going this way (i.e. you still need a lot of changes for large object allocator), please let me know and I will look more into your code.

[k-sareen]

I believe all your concerns should have been met. I have been unable to test the interaction of stress testing with large object space, but since the count is updated in alloc_slow_inline() now, it should work.

EDIT: Apologies for the messy git log. If possible, please squash the formatting commits during merge. I'll set up a reminder to run the ci-style script before every commit

[qinsoon]

I believe all your concerns should have been met. I have been unable to test the interaction of stress testing with large object space, but since the count is updated in alloc_slow_inline() now, it should work.

The race condition still exists. As long as reading and updating allocation_bytes is not atomic, there is a race condition. It would end up allocating more bytes than the defined stress factor before triggering a GC. To solve this, you would need to refactor to put reading and updating code together and make it atomic. You can refer to this: https://stackoverflow.com/questions/47753528/how-to-compare-and-increment-an-atomic-variable

However, I think this PR is good to merge, and that race issue can be resolved in a separate PR.

EDIT: Apologies for the messy git log. If possible, please squash the formatting commits during merge. I'll set up a reminder to run the ci-style script before every commit

Don't worry about that. We always do squash and merge.

[javadamiri]

LGTM