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.

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

Remove NULL ObjectReference #1064

Merged
merged 16 commits into from
Apr 27, 2024
Merged

Remove NULL ObjectReference #1064

merged 16 commits into from
Apr 27, 2024

Conversation

wks
Copy link
Collaborator

@wks wks commented Jan 8, 2024

We changed ObjectReference so that it is backed by NonZeroUsize, and can no longer represent a NULL reference. For cases where an ObjectReferences may or may not exist, Option<ObjectReference> should be used instead.

This is an API-breaking change.

  • ObjectReference::NULL and ObjectReference::is_null() are removed.
  • ObjectReference::from_raw_address() now returns Option<ObjectReference>. The unsafe method ObjectReference::from_raw_address_unchecked() assumes the address is not zero, and returns ObjectReference.
  • API functions that may or may not return a valid ObjectReference now return Option<ObjectReference>. Examples include Edge::load() and ReferenceGlue::get_referent().

If a VM binding needs to expose Option<ObjectReference> to native (C/C++) programs, a convenient type crate::util::api_util::NullableObjectReference is provided.

Fixes: #1043

@wks
Copy link
Collaborator Author

wks commented Jan 19, 2024

Here are some preliminary benchmark results. Two builds:

lusearch from DaCapo Chopin, 40 invocations, 5 iterations, 2.5x and 3x min heap size, ran on shrew.moma (Coffee Lake) and vole.moma (Ryzen 4).

I ran it with Immix and GenCopy. However GenCopy overflowed the phase counter. As a result, the stats of subsequent GCs are discarded from the result. (See #1074) So I am only showing the results for Immix.

image

https://squirrel.anu.edu.au/plotty/wks/noproject/#0|shrew-2024-01-19-Fri-080239^shrew-2024-01-19-Fri-113438^vole-2024-01-19-Fri-080218^vole-2024-01-19-Fri-113411&benchmark^build^hfac^invocation^iteration^logfile^mmtk_gc&GC^time^time.other^time.stw&|10&iteration^1^4&mmtk_gc^1^Immix|20&1^invocation|30&1&benchmark^hfac^logfile&build;build1|60&hfac&logfile|70&hfac-logfile^shrew-vole-253x|40&Histogram%20(with%20CI)^build^hfac-logfile&

From the data, the impact on total time is between -0.5% to +0.5%, and the error bar is large. Interestingly, it always shows slowdown on shrew, and always speed-up on vole. For STW time, "shrew 2.5x" has a slow-down of 1%, and "vole 3x" has a speed-up of 1.5%, but their error bars are too large.

I think the result of forcing ObjectReference to be non-zero is insignificant for lusearch with Immix.

@wks
Copy link
Collaborator Author

wks commented Jan 22, 2024

More benchmark results. I ran the same binary on shrew.moma and mole.moma (identical to vole.moma), 20 invocations, 3x min heap size, using Immix and StickyImmix GC.

Plotty link: https://squirrel.anu.edu.au/plotty/wks/noproject/#0|mole-2024-01-20-Sat-113636^shrew-2024-01-20-Sat-113610&benchmark^build^invocation^iteration^logfile^mmtk_gc&GC^time^time.other^time.stw&|10&iteration^1^4|20&1^invocation|30&1&benchmark^logfile^mmtk_gc&build;build1|60&build&logfile&mmtk_gc|70&build-logfile-mmtk_gc^blg-2024-01-22|41&Histogram%20(with%20CI)^build-logfile-mmtk_gc^benchmark&

Total time: normalized to build1 of the corresponding machine-plan pair:
image

For each machine-plan pair, the geomean of total time of build2 is between 99.6% to 100.3% of that of build1.

Other time:
image

Geomeans are between 99.8% to 100.3% of build1.

STW time:
image

STW times are between 97.9% to 101.0% of build1. But given the noise level, it is hard to conclude there is any improvement. We can see significant speed-up for build2-shrew-StickyImmix for biojava, jython and pmd, but build2-mole-StickyImmix does not have the same speed-up for those benchmarks. This is a bit hard to explain.

wks added a commit to wks/mmtk-jikesrvm that referenced this pull request Apr 19, 2024
wks added a commit to wks/mmtk-ruby that referenced this pull request Apr 24, 2024
wks added a commit to wks/mmtk-ruby that referenced this pull request Apr 24, 2024
wks added a commit to wks/mmtk-v8 that referenced this pull request Apr 24, 2024
wks added a commit to wks/mmtk-julia that referenced this pull request Apr 24, 2024
@wks
Copy link
Collaborator Author

wks commented Apr 24, 2024

binding-refs
OPENJDK_BINDING_REPO=wks/mmtk-openjdk
OPENJDK_BINDING_REF=fix/no-null-objref
JIKESRVM_BINDING_REPO=wks/mmtk-jikesrvm
JIKESRVM_BINDING_REF=fix/no-null-objref
V8_BINDING_REPO=wks/mmtk-v8
V8_BINDING_REF=fix/no-null-objref
RUBY_BINDING_REPO=wks/mmtk-ruby
RUBY_BINDING_REF=fix/no-null-objref
JULIA_BINDING_REPO=wks/mmtk-julia
JULIA_BINDING_REF=fix/no-null-objref

@wks wks added the PR-extended-testing Run extended tests for the pull request label Apr 24, 2024
@wks wks marked this pull request as ready for review April 24, 2024 16:48
@wks wks requested a review from qinsoon April 24, 2024 16:49
@wks
Copy link
Collaborator Author

wks commented Apr 24, 2024

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. There are some minor points.

Also a quick check: is there any change that may affect performance for OpenJDK and MMTk since the evaluation in #1064 (comment)?

@@ -479,28 +480,40 @@ use crate::vm::VMBinding;
/// the opaque `ObjectReference` type, and we haven't seen a use case for now.
#[repr(transparent)]
#[derive(Copy, Clone, Eq, Hash, PartialOrd, Ord, PartialEq, NoUninit)]
pub struct ObjectReference(usize);
pub struct ObjectReference(NonZeroUsize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments for the type should note that ObjectReference cannot be null, and for places where we may have an invalid object reference, we use Option<ObjectReference>. NonZeroUsize makes sure that Option<ObjectReference> has the same length as ObjectReference.

src/util/mod.rs Outdated
@@ -11,6 +11,8 @@ pub mod address;
/// Allocators
// This module is made public so the binding could implement allocator slowpaths if they would like to.
pub mod alloc;
/// Helpers for making native APIs.
pub mod apiutils;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use the name api_util to be consistent with other existing modules such as rust_util/test_util.

@wks
Copy link
Collaborator Author

wks commented Apr 26, 2024

Also a quick check: is there any change that may affect performance for OpenJDK and MMTk since the evaluation in #1064 (comment)?

I don't think so, but I'll re-run the benchmarks just to be safe.

@wks
Copy link
Collaborator Author

wks commented Apr 27, 2024

I re-ran the benchmarks with the same setting as in #1064 (comment)

Plotty: https://squirrel.anu.edu.au/plotty/wks/noproject/#0|shrew-2024-04-26-Fri-091850^mole-2024-04-26-Fri-091914&benchmark^build^invocation^iteration^logfile^mmtk_gc&GC^time^time.other^time.stw&|10&iteration^1^4|20&1^invocation|30&1&benchmark^logfile^mmtk_gc&build;build1|60&build&mmtk_gc&logfile|70&build-mmtk_gc-logfile^mole-shrew-20240426|40&Histogram%20(with%20CI)^build-mmtk_gc-logfile^benchmark&

Total time: (geomens of build2 are between 99.5% to 100.2% of build1, varying among plan * machine pairs)

image

Other time: (99.6% to 100.1%)

image

STW time: (99.2% to 100.8%)

image

The differences are within 1%. This result is consistent with the last evaluation.

There were a few comments that still mentioned null ObjectReference.
They have been removed.
@wks wks enabled auto-merge April 27, 2024 13:09
@wks wks added this pull request to the merge queue Apr 27, 2024
Merged via the queue into mmtk:master with commit a02803b Apr 27, 2024
22 of 24 checks passed
@wks wks deleted the fix/no-null-objref branch April 27, 2024 14:30
mmtkgc-bot added a commit to mmtk/mmtk-v8 that referenced this pull request Apr 27, 2024
Parent PR: mmtk/mmtk-core#1064

---------

Co-authored-by: mmtkgc-bot <[email protected]>
mmtkgc-bot added a commit to mmtk/mmtk-ruby that referenced this pull request Apr 27, 2024
Parent PR: mmtk/mmtk-core#1064

---------

Co-authored-by: mmtkgc-bot <[email protected]>
mmtkgc-bot added a commit to mmtk/mmtk-openjdk that referenced this pull request Apr 27, 2024
Upstream PR: mmtk/mmtk-core#1064

---------

Co-authored-by: mmtkgc-bot <[email protected]>
mmtkgc-bot added a commit to mmtk/mmtk-jikesrvm that referenced this pull request Apr 27, 2024
Upstream PR: mmtk/mmtk-core#1064

---------

Co-authored-by: mmtkgc-bot <[email protected]>
mmtkgc-bot added a commit to mmtk/mmtk-julia that referenced this pull request Apr 27, 2024
Parent PR: mmtk/mmtk-core#1064

---------

Co-authored-by: mmtkgc-bot <[email protected]>
qinsoon pushed a commit to qinsoon/mmtk-julia that referenced this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove NULL ObjectReference
2 participants