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

Support custom threads #415

Merged
merged 173 commits into from
Jan 9, 2025
Merged

Support custom threads #415

merged 173 commits into from
Jan 9, 2025

Conversation

eupp
Copy link
Collaborator

@eupp eupp commented Oct 11, 2024

No description provided.

@eupp eupp marked this pull request as ready for review October 12, 2024 01:30
@eupp eupp requested a review from ndkoval October 12, 2024 01:30
@eupp
Copy link
Collaborator Author

eupp commented Oct 12, 2024

The only failing CI configuration is "Integration Test with kotlinx.coroutines".
I believe it is because of #412, and once we merge #413 the problem should be fixed.
I've create a separate test branch where I merged the #413 into my branch, and it seems that there all kotlinx.coroutines tests are passed.

@eupp eupp force-pushed the dynamic-threads branch 2 times, most recently from 0ad50bd to 25c0c45 Compare October 31, 2024 17:15
@eupp eupp requested a review from ndkoval October 31, 2024 17:20
@eupp
Copy link
Collaborator Author

eupp commented Oct 31, 2024

@ndkoval while working on this I discovered a few problems, which are probably should be addressed in separate PRs.

One of the problems is related to the local objects tracking --- the current implementation does not work correctly with custom threads (see example below and the comment in DataStructuresTests::incorrectHashMap test).
So the problem can be illustrated by the following example:

class Box(var x: Int)

fun test(): Int {
    val box = Box() // <- this object is incorrectly classified as a local object
    thread {
        box.x = 42 // the local object tracker does not detect here that the `box` object,
                   // stored in the local variable, escapes into another thread;
                   // thus it will not insert a switch point before accesses to this object fields
    }
    return box.x
}

I would propose that we can address this problem separately in another PR after we merge this one.
The reasons is that adding support of this case would require significant refactoring of local objects tracking algorithm,
and this PR is already big.

Alternatively, we can first perform the necessary refactoring of the local objects tracking algorithm in a separate PR,
and after this rebase and merge custom threads PR.

@eupp
Copy link
Collaborator Author

eupp commented Nov 26, 2024

Another small bug fix on which this PR relies on: #426

gradle.properties Outdated Show resolved Hide resolved
bootstrap/src/sun/nio/ch/lincheck/Injections.java Outdated Show resolved Hide resolved
bootstrap/src/sun/nio/ch/lincheck/Injections.java Outdated Show resolved Hide resolved
bootstrap/src/sun/nio/ch/lincheck/Injections.java Outdated Show resolved Hide resolved
src/jvm/main/org/jetbrains/kotlinx/lincheck/IdeaPlugin.kt Outdated Show resolved Hide resolved
src/jvm/main/org/jetbrains/kotlinx/lincheck/util/Utils.kt Outdated Show resolved Hide resolved
@eupp
Copy link
Collaborator Author

eupp commented Dec 12, 2024

From what I can see, there is no significant changes in the time of CI builds between this branch and develop:

The reason is that in this PR I strive to preserve the old behavior whenever possible, by adding special treatment of TestThread class, see an example here:

if (thread instanceof TestThread) {

Thus, all the code interacting with TestThread-s (i.e. threads created by Lincheck) should work almost the same as before, and there should be no new performance penalties for it.

@eupp eupp requested a review from ndkoval December 13, 2024 20:43
bootstrap/src/sun/nio/ch/lincheck/ThreadRegistry.java Outdated Show resolved Hide resolved
src/jvm/main/org/jetbrains/kotlinx/lincheck/util/Utils.kt Outdated Show resolved Hide resolved
val elapsedTime = threadData.spinner.spinWaitTimedUntil(timeoutNano) {
threadData.state == ThreadState.FINISHED ||
// TODO: due to limitations of current implementation,
// Lincheck test threads sometime end up in ABORTED state,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can this happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remember correctly, onThreadFinish is not guaranteed to be called at the end of the thread execution.
This can happen in case of timeout, or in case if some exception was thrown from the code and not properly handled.

To fix this, we will need to modify TestThreadExecutionGenerator class, to modify the generated bytecode. Wrap the code in try-catch, and put onThreadFinish into finally block to guarantee it is always called at the end.

@eupp eupp requested a review from ndkoval January 8, 2025 10:35
eupp added 27 commits January 9, 2025 13:40
Signed-off-by: Evgeniy Moiseenko <[email protected]>
* do not switch to a thread awaiting thread join until the awaited thread finishes
* print the awaited thread id in the thread switch trace point

Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
… of ignored section check)

Signed-off-by: Evgeniy Moiseenko <[email protected]>
… of ignored section check)

Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
… `utils/Ensure.kt`

Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp eupp force-pushed the dynamic-threads branch from 2f51a95 to 8b4bf1b Compare January 9, 2025 12:53
@ndkoval ndkoval merged commit a3523c9 into develop Jan 9, 2025
16 checks passed
@ndkoval ndkoval deleted the dynamic-threads branch January 9, 2025 17:40
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.

2 participants