Skip to content
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

Bug fix: assertion fail when allocating for sizes that are not a multiple of MIN_ALIGNMENT #140

Merged
merged 6 commits into from Oct 5, 2020
Merged

Bug fix: assertion fail when allocating for sizes that are not a multiple of MIN_ALIGNMENT #140

merged 6 commits into from Oct 5, 2020

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Sep 25, 2020 •
edited

This PR fixes #139.

  • Added a test to reproduce the issue.
  • Added a new constant ALLOC_END_ALIGNMENT for this assertion. It is incorrect to use MIN_ALIGNMENT for the assertion.
  • Moved alignment constants to VMBinding so that they are VM specific with default values.
  • Added to ci-test.sh to run the tests from DummyVM.
  • Fixed an issue in example/build.py that is inconsistent about the env var RUSTUP_TOOLCHAIN.

@qinsoon qinsoon force-pushed the issue139-alloc-alignment-assertion branch from 5537472 to e77426a Compare September 25, 2020 01:40
caizixian
caizixian requested changes Sep 26, 2020
View reviewed changes
.github/scripts/ci-test.sh Outdated Show resolved Hide resolved
.github/workflows/pre-review-ci.yml Outdated Show resolved Hide resolved
src/util/alloc/allocator.rs Show resolved Hide resolved
@caizixian caizixian added the PR-under-review Pull request currently in review process label Sep 26, 2020
@caizixian
Copy link
Member

Logically, should we put the tests under dummy vm?

@qinsoon
Copy link
Member Author

qinsoon commented Sep 26, 2020

Logically, should we put the tests under dummy vm?

We probably will move DummyVM to the core crate (and rename it to MockVM). However, at this point, I don't know if there is a better place to put those tests.

@caizixian
Copy link
Member

Logically, should we put the tests under dummy vm?

We probably will move DummyVM to the core crate (and rename it to MockVM). However, at this point, I don't know if there is a better place to put those tests.

We don't have to fix this for this PR. But we should have an issue opened.

@qinsoon
Copy link
Member Author

qinsoon commented Sep 26, 2020

Logically, should we put the tests under dummy vm?

We probably will move DummyVM to the core crate (and rename it to MockVM). However, at this point, I don't know if there is a better place to put those tests.

We don't have to fix this for this PR. But we should have an issue opened.

Yeah, we have one: #99

@caizixian
Copy link
Member

Two points from the meeting today

  • We can assume different object end address alignment for different VMs, so the constant should be VM dependent
  • The MIN_ALIGNMENT should be used for minimal alignment for the object start. We need to use a different constant name for the purpose of this issue.

@qinsoon qinsoon force-pushed the issue139-alloc-alignment-assertion branch from 5265786 to 70ff9fb Compare September 29, 2020 01:03
@qinsoon
Copy link
Member Author

qinsoon commented Sep 29, 2020

Two points from the meeting today

  • We can assume different object end address alignment for different VMs, so the constant should be VM dependent
  • The MIN_ALIGNMENT should be used for minimal alignment for the object start. We need to use a different constant name for the purpose of this issue.

I have rebased this PR with master and fixed both points. Now those constants are moved to VMBinding to be VM-specific with default values. A new constant ALLOC_END_ALIGNMENT is introduced for this specific assertion.

@github-actions
Copy link

OpenJDK

SemiSpace (wrench-2020-09-29-Tue-112636)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 401.35 ±3.47 ⚠️ 6/40 failed 401.35 ±3.47 402.5 401.53 ±4.45 ⚠️ 4/40 failed 401.53 ±4.45 403.0 +0.04% +0.04%
eclipse 7653.07 ±33.84 7653.07 ±33.84 7699.0 7658.11 ±36.09 ⚠️ 4/40 failed 7658.11 ±36.09 7702.0 +0.07% +0.07%
fop 464.12 ±2.48 463.18 ±1.63 ⚠️ 1 removed 462.0 461.62 ±0.90 461.38 ±0.78 ⚠️ 1 removed 461.0 -0.54% -0.39%
hsqldb 1001.02 ±12.79 1001.02 ±12.79 1010.5 1002.33 ±13.28 1002.33 ±13.28 1010.5 +0.13% +0.13%
pmd 1343.45 ±7.67 1343.45 ±7.67 1341.0 1344.17 ±4.90 1344.17 ±4.90 1342.5 +0.05% +0.05%

@qinsoon
Copy link
Member Author

qinsoon commented Sep 29, 2020

jikesrvm-binding-test
JIKESRVM_BINDING_REF=issue139-alloc-alignment-assertion

@qinsoon
Copy link
Member Author

qinsoon commented Sep 29, 2020

jikesrvm-perf-compare
JIKESRVM_BINDING_BRANCH_REF=issue139-alloc-alignment-assertion

@github-actions
Copy link

JikesRVM

NoGC (wrench-2020-09-29-Tue-141726)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 794.62 ±3.75 793.31 ±2.71 ⚠️ 1 removed 792.5 790.75 ±2.88 790.75 ±2.88 792.0 -0.49% -0.32%
fop 627.80 ±1.34 627.80 ±1.34 628.0 628.15 ±1.56 628.15 ±1.56 628.0 +0.06% +0.06%
luindex 2325.80 ±6.58 2323.77 ±5.28 ⚠️ 1 removed 2325.5 2316.67 ±5.66 ⚠️ 1/40 failed 2316.67 ±5.66 2318.0 -0.39% -0.31%

SemiSpace (wrench-2020-09-29-Tue-142935)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 651.05 ±7.84 646.34 ±4.32 ⚠️ 2 removed 648.5 640.49 ±3.83 ⚠️ 1/40 failed 639.26 ±3.00 ⚠️ 1 removed 637.0 -1.62% -1.10% 🟩
bloat 2422.33 ±28.78 ⚠️ 1/40 failed 2422.33 ±28.78 2406.0 2439.65 ±25.33 2439.65 ±25.33 2453.5 +0.71% +0.71%
fop 590.17 ±1.25 590.17 ±1.25 590.0 589.08 ±1.28 588.77 ±1.15 ⚠️ 1 removed 589.0 -0.19% -0.24%
hsqldb 1155.55 ±17.08 1155.55 ±17.08 1158.0 1164.20 ±17.35 1164.20 ±17.35 1170.0 +0.75% +0.75%
jython 1816.83 ±8.11 1816.83 ±8.11 1815.0 1824.33 ±8.36 1824.33 ±8.36 1825.0 +0.41% +0.41%
luindex 2368.32 ±8.68 ⚠️ 2/40 failed 2368.32 ±8.68 2368.5 2358.22 ±9.42 2358.22 ±9.42 2361.0 -0.43% -0.43%
lusearch 674.97 ±3.07 ⚠️ 1/40 failed 674.18 ±2.70 ⚠️ 1 removed 673.0 675.82 ±2.84 ⚠️ 1/40 failed 675.11 ±2.51 ⚠️ 1 removed 673.0 +0.13% +0.14%
pmd 1474.53 ±9.31 1474.53 ±9.31 1472.0 1479.53 ±8.37 1479.53 ±8.37 1479.5 +0.34% +0.34%
xalan 504.40 ±2.75 504.40 ±2.75 503.0 508.95 ±2.84 508.95 ±2.84 509.0 +0.90% +0.90%

@github-actions
Copy link

OpenJDK

SemiSpace (wrench-2020-09-29-Tue-160440)

Benchmark Trunk(ms) Branch(ms) Diff
mean mean without outliers median mean mean without outliers median mean mean without outliers
antlr 398.56 ±5.58 ⚠️ 8/40 failed 398.56 ±5.58 402.0 397.36 ±5.27 ⚠️ 4/40 failed 397.36 ±5.27 402.0 -0.30% -0.30%
eclipse 7670.68 ±34.58 ⚠️ 2/40 failed 7670.68 ±34.58 7715.5 7644.57 ±33.57 7644.57 ±33.57 7683.5 -0.34% -0.34%
fop 466.27 ±4.03 463.63 ±1.60 ⚠️ 2 removed 463.0 463.90 ±2.40 462.39 ±1.17 ⚠️ 2 removed 461.5 -0.51% -0.27%
hsqldb 998.62 ±12.08 998.62 ±12.08 1007.5 1001.27 ±13.65 1001.27 ±13.65 1010.5 +0.27% +0.27%
pmd 1339.83 ±7.08 1339.83 ±7.08 1342.5 1349.05 ±6.80 1349.05 ±6.80 1348.0 +0.69% +0.69%

@qinsoon
Copy link
Member Author

qinsoon commented Oct 1, 2020

As mmtk/mmtk-jikesrvm#24 is merged, we should merge this one as soon as we can. I believe the requested changes are addressed, and we can merge this. @caizixian

@caizixian caizixian merged commit a0a79fd into master Oct 5, 2020
@caizixian caizixian deleted the issue139-alloc-alignment-assertion branch October 5, 2020 23:10
@qinsoon qinsoon mentioned this pull request Jan 5, 2023
Open
3 tasks
qinsoon added a commit to qinsoon/mmtk-core that referenced this pull request Mar 28, 2023
@qinsoon
Remove VM submodule (mmtk#140)
18d35d8 
This PR removes the git submodule for OpenJDK. This PR closes mmtk/mmtk-openjdk#121.

Changes:
* Removed OpenJDK git submodule. The metadata about the OpenJDK version is now stored in `mmtk/Cargo.toml`.
* Added `common.sh` to `.github/scripts`, which is included by each CI shell script.
* Added `ci-checkout.sh` to `.github/scripts` that checks out the correct OpenJDK version for CI.
* Fixed paths in `CompileThirdPartyHeap.gmk`. We no longer assume OpenJDK is in `repos/openjdk`.
* Updated `README`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caizixian caizixian caizixian approved these changes

Assignees
No one assigned
Labels
PR-under-review Pull request currently in review process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allocating objects of sizes that are not a multiple of MIN_ALIGNMENT causes assertion fail
2 participants
@qinsoon @caizixian