Skip to content

[vm] AppJIT snapshot loses ImmutableBit of objects #55136

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

Closed
dcharkes opened this issue Mar 7, 2024 · 2 comments
Closed

[vm] AppJIT snapshot loses ImmutableBit of objects #55136

dcharkes opened this issue Mar 7, 2024 · 2 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. triaged Issue has been triaged by sub team

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Mar 7, 2024

I believe we don't set the ImmutableBit when deserializing from appjit snapshots.

void Deserializer::InitializeHeader(ObjectPtr raw,
intptr_t class_id,
intptr_t size,
bool is_canonical) {
ASSERT(Utils::IsAligned(size, kObjectAlignment));
uword tags = 0;
tags = UntaggedObject::ClassIdTag::update(class_id, tags);
tags = UntaggedObject::SizeTag::update(size, tags);
tags = UntaggedObject::CanonicalBit::update(is_canonical, tags);
tags = UntaggedObject::AlwaysSetBit::update(true, tags);
tags = UntaggedObject::NotMarkedBit::update(true, tags);
tags = UntaggedObject::OldAndNotRememberedBit::update(true, tags);
tags = UntaggedObject::NewBit::update(false, tags);
raw->untag()->tags_ = tags;
}

This leads to objects being copied instead of being shared on SendPort.send.

cc @mkustermann

Edit: Can be exercised with https://dart-review.googlesource.com/c/sdk/+/354902/17..18 and tools/build.py -mdebug create_platform_sdk runtime && tools/test.py -n vm-appjit-linux-debug-x64 lib/isolate/illegal_msg_mirror_test.

@dcharkes dcharkes added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Mar 7, 2024
@dcharkes dcharkes assigned dcharkes and unassigned dcharkes Mar 7, 2024
@dcharkes
Copy link
Contributor Author

dcharkes commented Mar 7, 2024

For the canonical bit, we make a separate cluster.

Maybe we should make separate clusters for the immutable bit as well.

WDYT @rmacnak-google ?

@rmacnak-google
Copy link
Contributor

The immutable bit is mostly of function of the class, so just initializing it like we do for normal allocation should be enough. (There are edge cases with immutable type data, but I think these can't occur at compile/const-time and end up in an app snapshot.) I don't think we need separate clusters until we starting having some instances of a class be immutable and others not.

@a-siva a-siva added the triaged Issue has been triaged by sub team label Mar 13, 2024
copybara-service bot pushed a commit that referenced this issue Mar 15, 2024
Currently the immutable bit is a of function of the class. If we start having some instances of a class be immutable and others not, we will need separate clusters like we do for the canonical bit.

TEST=vm/dart/snapshot_immutable_bit_test
Bug: #55136
Cq-Include-Trybots: luci.dart.try:vm-appjit-linux-debug-x64-try
Change-Id: I073f28cabca8293b766aa7c0b224934a62bf9cb4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/356280
Reviewed-by: Daco Harkes <[email protected]>
Commit-Queue: Ryan Macnak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants