Skip to content

Conversation

@wenyuzhao
Copy link
Member

@wenyuzhao wenyuzhao commented Jun 9, 2021

This PR adds a PageProtect GC, which

  1. Allocates each object on a separate page.
  2. Protect pages on release, and unprotect on re-allocate. -- useful for debugging.

Note:
Currently it only supports OpenJDK. Support for V8 and JikesRVM is subject to be done in future PRs.

@wenyuzhao wenyuzhao requested review from caizixian and qinsoon June 9, 2021 13:49
@wenyuzhao wenyuzhao marked this pull request as ready for review June 9, 2021 13:49
@qinsoon
Copy link
Member

qinsoon commented Jun 9, 2021

Before a detailed review, there are a few general issues I think should be addressed:

  1. Binding support. Theoretically, our plans should be supported by all the bindings if possible. In case you need any help to implement the plan for the bindings, you can ask Javad or me to help. For this PR,
    • For OpenJDK. As you mentioned, you have got OpenJDK working for PageProtect. There should be a PR for the PageProtect plan with tests.
    • For JikesRVM, to support this plan, we should only need minimal changes. If there are bugs or other issues that blocks it, please make it clear.
    • For V8, as there will be a large amount of code to properly support GC in V8, I think we do not need to support PageProtect in V8 in this PR.
  2. The hack in build.py test to skip tests for PageProtect for 32 bits. The tests ensure new code is tested. Generally we do not modify our tests unless the test is wrong. This plan is not architecture dependent, and it should work on 32bits. If for some reason, it cannot run on 32 bits, it means either we have a bug somewhere or the plan is not implemented correctly. Please diagnose on this further.

@wenyuzhao
Copy link
Member Author

Before a detailed review, there are a few general issues I think should be addressed:

  1. Binding support. Theoretically, our plans should be supported by all the bindings if possible. In case you need any help to implement the plan for the bindings, you can ask Javad or me to help. For this PR,

    • For OpenJDK. As you mentioned, you have got OpenJDK working for PageProtect. There should be a PR for the PageProtect plan with tests.
    • For JikesRVM, to support this plan, we should only need minimal changes. If there are bugs or other issues that blocks it, please make it clear.
    • For V8, as there will be a large amount of code to properly support GC in V8, I think we do not need to support PageProtect in V8 in this PR.
  2. The hack in build.py test to skip tests for PageProtect for 32 bits. The tests ensure new code is tested. Generally we do not modify our tests unless the test is wrong. This plan is not architecture dependent, and it should work on 32bits. If for some reason, it cannot run on 32 bits, it means either we have a bug somewhere or the plan is not implemented correctly. Please diagnose on this further.

The reason why JikesRVM is unsupported and the hack in build.py is that this GC triggers a bug and crash in IntArrayFreeList under 32bit build... I'll take time to fix this.

I'm afraid this PR may block the next mmtk release. If this is the case, perhaps we can merge this first without JikesRVM support, and do it in another PR.

For V8 support, I've made it working already, but with more hacks to the mmtk-core and binding. I'll do more cleanup and fix the v8 support in future PRs.

@qinsoon
Copy link
Member

qinsoon commented Jun 9, 2021

Before a detailed review, there are a few general issues I think should be addressed:

  1. Binding support. Theoretically, our plans should be supported by all the bindings if possible. In case you need any help to implement the plan for the bindings, you can ask Javad or me to help. For this PR,

    • For OpenJDK. As you mentioned, you have got OpenJDK working for PageProtect. There should be a PR for the PageProtect plan with tests.
    • For JikesRVM, to support this plan, we should only need minimal changes. If there are bugs or other issues that blocks it, please make it clear.
    • For V8, as there will be a large amount of code to properly support GC in V8, I think we do not need to support PageProtect in V8 in this PR.
  2. The hack in build.py test to skip tests for PageProtect for 32 bits. The tests ensure new code is tested. Generally we do not modify our tests unless the test is wrong. This plan is not architecture dependent, and it should work on 32bits. If for some reason, it cannot run on 32 bits, it means either we have a bug somewhere or the plan is not implemented correctly. Please diagnose on this further.

The reason why JikesRVM is unsupported and the hack in build.py is that this GC triggers a bug and crash in IntArrayFreeList under 32bit build... I'll take time to fix this.

I'm afraid this PR may block the next mmtk release. If this is the case, perhaps we can merge this first without JikesRVM support, and do it in another PR.

For V8 support, I've made it working already, but with more hacks to the mmtk-core and binding. I'll do more cleanup and fix the v8 support in future PRs.

The bug about IntArrayFreeList may be the one that I am looking at as well. Is it related with the new_chunk change in this PR? I am concerned that simply removing that new_chunk line is incorrect. Digging further into this may block or delay the release, but unless we understand more about the bug, I don't feel confident that this PR is correct.

@qinsoon
Copy link
Member

qinsoon commented Jun 24, 2021

openjdk-binding-test
OPENJDK_BINDING_REF=paging-gc

@qinsoon
Copy link
Member

qinsoon commented Jun 24, 2021

openjdk-perf-compare
OPENJDK_BINDING_BRANCH_REF=paging-gc

@qinsoon qinsoon mentioned this pull request Jun 25, 2021
@github-actions
Copy link

OpenJDK Micro Benchmarks

Running: ['rebench', 'microbm.conf', 'CI_SemiSpace']

Benchmark Trunk (ms) Branch (ms) Diff
BinaryTrees 1909.39 1912.30 +0.15%
Fasta 934.98 936.80 +0.19%
GCBenchMT 1435.83 1435.99 +0.01%
GCBenchST 108.15 108.25 +0.09%
ReverseComplement 52.03 52.39 +0.68%

@github-actions
Copy link

OpenJDK

SemiSpace (spanner-2021-06-26-Sat-234713)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 421.50 ±1.86 ⚠️ 2/40 failed 421.50 ±1.86 422.0 421.74 ±0.58 ⚠️ 2/40 failed 421.74 ±0.58 422.0 +0.06% +0.06%
eclipse 7952.25 ±53.06 7932.82 ±36.61 ⚠️ 1 removed 7900.5 7927.32 ±37.06 7927.32 ±37.06 7897.5 -0.31% -0.07%
fop 508.88 ±3.07 508.88 ±3.07 505.0 506.98 ±2.38 506.33 ±2.05 ⚠️ 1 removed 505.0 -0.37% -0.50%
hsqldb 489.48 ±12.24 489.48 ±12.24 474.0 498.77 ±14.25 498.77 ±14.25 477.0 +1.90% +1.90% 🟥
pmd 1403.47 ±9.16 1401.15 ±8.07 ⚠️ 1 removed 1398.0 1399.42 ±8.97 1399.42 ±8.97 1398.0 -0.29% -0.12%

GenCopy (spanner-2021-06-27-Sun-020156)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 442.20 ±0.74 442.20 ±0.74 442.0 442.10 ±0.81 442.10 ±0.81 442.0 -0.02% -0.02%
eclipse 8652.10 ±43.98 ⚠️ 1/40 failed 8639.32 ±36.53 ⚠️ 1 removed 8666.0 8663.55 ±35.89 8663.55 ±35.89 8686.5 +0.13% +0.28%
fop 510.70 ±1.14 510.70 ±1.14 511.0 510.38 ±0.99 510.38 ±0.99 510.5 -0.06% -0.06%
hsqldb 403.70 ±3.67 403.70 ±3.67 406.5 405.07 ±3.83 405.07 ±3.83 406.5 +0.34% +0.34%
pmd 1655.87 ±20.44 ⚠️ 1/40 failed 1655.87 ±20.44 1633.0 1662.78 ±22.13 1662.78 ±22.13 1641.5 +0.42% +0.42%

@javadamiri javadamiri added PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) PR-benchmarking and removed PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) labels Jun 27, 2021
@javadamiri
Copy link
Contributor

javadamiri commented Jun 27, 2021

jikesrvm-perf-compare
JIKESRVM_BINDING_BRANCH_REF=pp_pr

@javadamiri
Copy link
Contributor

jikesrvm-binding-test
JIKESRVM_BINDING_REF=pp_pr

@github-actions
Copy link

JikesRVM

NoGC (wrench-2021-06-27-Sun-131958)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 774.62 ±2.64 774.62 ±2.64 774.0 780.17 ±3.53 780.17 ±3.53 780.0 +0.72% +0.72%
fop 629.90 ±1.55 629.90 ±1.55 628.5 631.10 ±1.44 631.10 ±1.44 630.0 +0.19% +0.19%
luindex 2275.39 ±6.01 ⚠️ 2/40 failed 2275.39 ±6.01 2277.5 2275.38 ±7.70 2273.33 ±6.67 ⚠️ 1 removed 2275.0 -0.00% -0.09%

SemiSpace (wrench-2021-06-27-Sun-133157)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 642.85 ±13.23 642.85 ±13.23 630.0 643.60 ±11.58 643.60 ±11.58 629.0 +0.12% +0.12%
bloat 2294.55 ±25.51 2294.55 ±25.51 2312.0 2288.95 ±26.44 2288.95 ±26.44 2286.5 -0.24% -0.24%
fop 592.75 ±1.09 592.75 ±1.09 592.0 593.45 ±1.29 593.45 ±1.29 593.5 +0.12% +0.12%
hsqldb 646.02 ±8.75 646.02 ±8.75 649.5 640.62 ±11.19 640.62 ±11.19 650.5 -0.84% -0.84%
jython 1696.28 ±10.27 1696.28 ±10.27 1697.0 1693.28 ±10.00 1693.28 ±10.00 1688.0 -0.18% -0.18%
luindex 2299.85 ±9.57 2299.85 ±9.57 2300.5 2300.60 ±6.48 2300.60 ±6.48 2295.5 +0.03% +0.03%
lusearch 619.40 ±15.11 615.44 ±13.15 ⚠️ 1 removed 606.5 587.83 ±12.80 587.83 ±12.80 589.5 -5.10% -4.49% 🟩
pmd 1255.38 ±6.99 1255.38 ±6.99 1256.5 1255.62 ±7.54 1255.62 ±7.54 1262.5 +0.02% +0.02%
xalan 516.27 ±3.49 516.27 ±3.49 517.0 513.17 ±3.44 513.17 ±3.44 515.0 -0.60% -0.60%

@github-actions
Copy link

OpenJDK

SemiSpace (wrench-2021-06-27-Sun-150240)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 407.73 ±12.55 404.54 ±11.06 ⚠️ 1 removed 392.5 400.79 ±8.43 ⚠️ 1/40 failed 400.79 ±8.43 390.0 -1.70% -0.93%
eclipse 7530.30 ±55.64 7485.05 ±22.91 ⚠️ 3 removed 7481.0 7488.85 ±32.44 7475.95 ±19.79 ⚠️ 1 removed 7480.5 -0.55% -0.12%
fop 479.10 ±2.99 477.08 ±0.82 ⚠️ 2 removed 477.0 479.23 ±2.09 477.92 ±1.03 ⚠️ 2 removed 477.5 +0.03% +0.18%
hsqldb 437.50 ±5.80 434.82 ±2.12 ⚠️ 1 removed 435.0 432.93 ±1.09 432.93 ±1.09 434.0 -1.05% -0.44%
pmd 1227.03 ±4.16 1227.03 ±4.16 1221.5 1229.60 ±4.45 1228.54 ±4.01 ⚠️ 1 removed 1227.5 +0.21% +0.12%

GenCopy (wrench-2021-06-27-Sun-170749)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 405.05 ±6.36 ⚠️ 1/40 failed 402.45 ±3.65 ⚠️ 1 removed 401.0 406.77 ±7.14 402.24 ±3.00 ⚠️ 2 removed 402.0 +0.43% -0.05%
eclipse 7919.70 ±18.24 7913.46 ±13.52 ⚠️ 1 removed 7922.5 7971.92 ±65.01 ⚠️ 1/40 failed 7937.76 ±46.04 ⚠️ 2 removed 7922.0 +0.66% +0.31%
fop 483.95 ±1.03 483.64 ±0.85 ⚠️ 1 removed 484.0 484.18 ±1.24 483.69 ±0.78 ⚠️ 1 removed 484.0 +0.05% +0.01%
hsqldb 369.80 ±2.77 369.80 ±2.77 372.0 369.35 ±2.65 369.35 ±2.65 371.5 -0.12% -0.12%
pmd 1459.38 ±17.71 1455.08 ±15.85 ⚠️ 1 removed 1452.5 1461.60 ±11.66 1461.60 ±11.66 1464.0 +0.15% +0.45%

@github-actions
Copy link

OpenJDK Micro Benchmarks

Running: ['rebench', 'microbm.conf', 'CI_SemiSpace']

Benchmark Trunk (ms) Branch (ms) Diff
BinaryTrees 1910.39 1914.21 +0.20%
Fasta 932.48 938.08 +0.60%
GCBenchMT 1434.51 1435.28 +0.05%
GCBenchST 107.97 108.06 +0.08%
ReverseComplement 52.59 52.55 -0.08%

@javadamiri javadamiri merged commit 62964b7 into master Jun 27, 2021
@javadamiri javadamiri deleted the paging-gc branch June 27, 2021 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants