Bug fix: assertion fail when allocating for sizes that are not a multiple of MIN_ALIGNMENT by qinsoon · Pull Request #140 · mmtk/mmtk-core

qinsoon · 2020-09-25T01:31:16Z

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.

.github/scripts/ci-test.sh

.github/workflows/pre-review-ci.yml

src/util/alloc/allocator.rs

caizixian · 2020-09-26T12:14:00Z

Logically, should we put the tests under dummy vm?

qinsoon · 2020-09-26T12:22:22Z

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 · 2020-09-26T12:27:26Z

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 · 2020-09-26T12:28:55Z

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 · 2020-09-28T04:08:56Z

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.

and keep the assertion.

qinsoon · 2020-09-29T01:21:07Z

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 · 2020-09-29T03:31:47Z

OpenJDK

binding_trunk: 038d0b47f275adf2d327914e087310afbc53a0bf
trunk: 6a70cb9dc7c949d1d714b30718dc8048341a2585
binding_branch: 038d0b47f275adf2d327914e087310afbc53a0bf
branch: 4af50fc25760a93041bdc53a62fe89d943991328

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 · 2020-09-29T04:12:18Z

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

qinsoon · 2020-09-29T04:13:00Z

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

github-actions · 2020-09-29T06:03:24Z

JikesRVM

binding_trunk: c8daf8d30e44f5532228f0db2dd60fda4cf06c8a
trunk: 6a70cb9dc7c949d1d714b30718dc8048341a2585
binding_branch: 09760fa503656fa1c0f1d439b9a4ce1a6cc136c9
branch: 4af50fc25760a93041bdc53a62fe89d943991328

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 · 2020-09-29T08:10:43Z

OpenJDK

binding_trunk: 038d0b47f275adf2d327914e087310afbc53a0bf
trunk: 6a70cb9dc7c949d1d714b30718dc8048341a2585
binding_branch: 038d0b47f275adf2d327914e087310afbc53a0bf
branch: 4af50fc25760a93041bdc53a62fe89d943991328

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 · 2020-10-01T05:42:08Z

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

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`.

qinsoon requested a review from caizixian September 25, 2020 01:31

qinsoon force-pushed the issue139-alloc-alignment-assertion branch from 5537472 to e77426a Compare September 25, 2020 01:40

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 added the PR-under-review Pull request currently in review process label Sep 26, 2020

qinsoon added 4 commits September 28, 2020 14:24

Add a test to reproduce the issue. Remove the assertion.

978c679

Fix workflow

e21537c

Remoev hard-coded plan list from ci-test.sh

d884db3

Move alignment constants as VMBinding constants. Add ALLOC_END_ALIGNMENT

70ff9fb

and keep the assertion.

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

qinsoon added 2 commits September 29, 2020 11:08

Fix rust version in pre-review-ci

3f4d795

cargo fmt

4af50fc

qinsoon added the PR-benchmarking label Sep 29, 2020

caizixian approved these changes Oct 5, 2020

View reviewed changes

caizixian merged commit a0a79fd into master Oct 5, 2020

caizixian deleted the issue139-alloc-alignment-assertion branch October 5, 2020 23:10

qinsoon mentioned this pull request Jan 5, 2023

Incorrect align_allocate and get_maximum_aligned_size #730

Open

3 tasks

Conversation

qinsoon commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caizixian commented Sep 26, 2020

Uh oh!

qinsoon commented Sep 26, 2020

Uh oh!

caizixian commented Sep 26, 2020

Uh oh!

qinsoon commented Sep 26, 2020

Uh oh!

caizixian commented Sep 28, 2020

Uh oh!

qinsoon commented Sep 29, 2020

Uh oh!

github-actions bot commented Sep 29, 2020

OpenJDK

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

Uh oh!

qinsoon commented Sep 29, 2020

Uh oh!

qinsoon commented Sep 29, 2020

Uh oh!

github-actions bot commented Sep 29, 2020

JikesRVM

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

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

Uh oh!

github-actions bot commented Sep 29, 2020

OpenJDK

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

Uh oh!

qinsoon commented Oct 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qinsoon commented Sep 25, 2020 •

edited

Loading