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

Static memory snapshot collecting and restoring #418

Merged
merged 29 commits into from
Dec 12, 2024

Conversation

dmitrii-artuhov
Copy link
Collaborator

@dmitrii-artuhov dmitrii-artuhov commented Oct 17, 2024

This PR partly resolves issue #389

Here we introduced a lazy-tracking feature which aims to only track and restore variables reachable from static memory iff the variables were accessed perviously via read or write operations. Yet, there are still problems (see issue) that are left to be tackled in separate PRs.

Proposed solutions in this PR:

  • lazy tracking for most of the variables
  • snapshot of collections are stored enegetically (meaning that the whole object graph is snapshotted). There are several APIs that are not handled yet, which does not allow a correct lazy snapshot building (e.g. System.arraycopy(...))
  • leaking this problem: I added a fix, which allows the test mentioned in the issue to pass, see. The TODO (calculating common superclass without loading additional classes to the VM) was resolved by leveraging SafeClassWriter::getCommonSuperClass(...). Thanks to @ndkoval for help!

@dmitrii-artuhov dmitrii-artuhov marked this pull request as draft October 17, 2024 23:28
@ndkoval ndkoval requested a review from eupp November 26, 2024 15:38
@ndkoval ndkoval changed the base branch from trace-debugger to develop November 26, 2024 16:16
@eupp
Copy link
Collaborator

eupp commented Nov 26, 2024

Let's rebase on develop (part of the diff should also disappear, because it is already merged into develop).

@dmitrii-artuhov dmitrii-artuhov force-pushed the trace-debugger-static-memory branch from 336e12d to 8eac5dc Compare November 27, 2024 14:36
@dmitrii-artuhov dmitrii-artuhov marked this pull request as ready for review November 27, 2024 16:07
@dmitrii-artuhov dmitrii-artuhov requested a review from eupp December 2, 2024 14:12
Copy link
Collaborator

@eupp eupp left a comment

Choose a reason for hiding this comment

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

@dmitrii-artuhov Please fix a few remaining minor issues, otherwise looks good to me.

src/jvm/main/org/jetbrains/kotlinx/lincheck/Utils.kt Outdated Show resolved Hide resolved
bootstrap/src/sun/nio/ch/lincheck/EventTracker.java Outdated Show resolved Hide resolved
}
return null

restoreStaticMemorySnapshot()
Copy link
Collaborator

Choose a reason for hiding this comment

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

But do you need additional call to restoreStaticMemorySnapshot here, if there is already a call to it in ManagedStrategy::run ?

@dmitrii-artuhov
Copy link
Collaborator Author

dmitrii-artuhov commented Dec 2, 2024

But do you need additional call to restoreStaticMemorySnapshot here, if there is already a call to it in ManagedStrategy::run?

@eupp I've rewritten it, see the modified Strategy.runIteration(...) function, please

@dmitrii-artuhov dmitrii-artuhov requested a review from eupp December 2, 2024 16:19
@@ -91,6 +90,14 @@ internal class LincheckClassVisitor(
}
if (methodName == "<init>") {
mv = ObjectCreationTransformer(fileName, className, methodName, mv.newAdapter())
mv = run {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we treat <init> similar to normal functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not try including <init> methods in the whole transformation chain. Right now I only require tracking fields/arrays accesses and other constructor invocations inside <init> method bodies.
Maybe the feature of integrating constructors in full transformation chain should be a separate PR, since there are might be tricky spots where to account for #424

@ndkoval ndkoval merged commit a736006 into develop Dec 12, 2024
16 checks passed
@ndkoval ndkoval deleted the trace-debugger-static-memory branch December 12, 2024 11:17
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.

3 participants