Support custom threads #415

eupp · 2024-10-11T01:39:04Z

No description provided.

eupp · 2024-10-12T02:17:08Z

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.

...vm/test/org/jetbrains/kotlinx/lincheck_test/transformation/KotlinStdlibTransformationTest.kt

eupp · 2024-10-31T17:34:54Z

@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 · 2024-11-26T17:51:33Z

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

…egy` Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

… blocking Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…ead group Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

* in preparation of implementing ignored sections for custom threads Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

… ignored sections Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…in `cancelByLincheck` Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…lass Signed-off-by: Evgeniy Moiseenko <[email protected]>

This reverts commit 2d52b3a.

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…in the thread descriptors map Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…n case of abort Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…y the output Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

…m stack traces) Signed-off-by: Evgeniy Moiseenko <[email protected]>

Signed-off-by: Evgeniy Moiseenko <[email protected]>

ndkoval · 2024-12-12T11:24:04Z

gradle.properties

@@ -15,7 +15,7 @@ version=2.35-SNAPSHOT
 inceptionYear=2019
 lastCopyrightYear=2023

-jdkToolchainVersion=17
+jdkToolchainVersion=8


please revert the change

ndkoval · 2024-12-12T11:25:09Z

bootstrap/src/sun/nio/ch/lincheck/Injections.java

@@ -45,6 +50,101 @@ public class Injections {
    @SuppressWarnings({"FieldCanBeLocal", "unused"})
    private static int currentEventId = -1;

+    // Thread local variable storing testing code and ignored section flags.


Please remove this line

ndkoval · 2024-12-12T11:26:35Z

bootstrap/src/sun/nio/ch/lincheck/Injections.java

+     * - These lists use copy-on-write strategy when a new descriptor is added to avoid race conditions.
+     * - Thread descriptors store weak references to thread objects, and thus do not prevent their garbage collection.
+     *
+     * TODO: although the garbage collection of thread objects is not prevented thanks to weak references,


You can do the cleanup on each 1000's (feel free choose any number here) access

ndkoval · 2024-12-12T11:27:33Z

bootstrap/src/sun/nio/ch/lincheck/Injections.java

+            return ((TestThread) thread).descriptor;
+        }
+        int hashCode = System.identityHashCode(thread);
+        ArrayList<ThreadDescriptor> threadDescriptors = threadDescriptorsMap.get(hashCode);


Can we use a weak hash map instead?

ndkoval · 2024-12-12T11:33:37Z

src/jvm/test/resources/expected_logs/obstruction_freedom_synchronized.txt

 |                                                                                                                          |   switch                                                                                                                 |
 | operation()                                                                                                              |                                                                                                                          |
-|   MONITORENTER at ObstructionFreedomSynchronizedRepresentationTest.operation(ObstructionFreedomRepresentationTest.kt:70) |                                                                                                                          |
+|   MONITORENTER at ObstructionFreedomSynchronizedRepresentationTest.operation(ObstructionFreedomRepresentationTest.kt:61) |                                                                                                                          |
+|   switch (reason: lock is already acquired)                                                                              |                                                                                                                          |


is there any switch? It seems the execution ends after this event.

ndkoval · 2024-12-12T11:38:30Z

src/jvm/test/resources/expected_logs/custom_threads_deque_trace.txt

+|   switch                                                                                                                                       |                                                                                                                                                             |          |
+|                                                                                                                                                | run()                                                                                                                                                       |          |
+|                                                                                                                                                |   switch                                                                                                                                                    |          |
+|   switch (reason: waiting for thread to join)                                                                                                  |                                                                                                                                                             |          |


let's mention which thread the execution is calling join on

ndkoval · 2024-12-12T11:42:55Z

src/jvm/test/org/jetbrains/kotlinx/lincheck_test/gpmc/Utils.kt

+internal fun modelCheckerTest(
+    testClass: KClass<*>,
+    testOperation: KFunction<*>,
+    outcomes: Set<Any?> = setOf(),


Are these the excepted outcomes?

ndkoval · 2024-12-12T11:43:48Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/IdeaPlugin.kt

@@ -79,7 +79,8 @@ fun shouldReplayInterleaving(): Boolean {
 */
 @Suppress("UNUSED_PARAMETER")
 fun beforeEvent(eventId: Int, type: String) {
-    val strategy = (Thread.currentThread() as? TestThread)?.eventTracker ?: return
+    val strategy = Injections.getCurrentThreadDescriptor()?.eventTracker


Have you checked how custom threads work with the plugin?

ndkoval · 2024-12-12T11:45:19Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/util/Utils.kt

+ * with this file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+package org.jetbrains.kotlinx.lincheck.util


Do we need to have to Utils.kt files?

ndkoval · 2024-12-12T11:50:45Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/LoopDetector.kt

@@ -204,7 +204,7 @@ internal class LoopDetector(
        // Has the thread changed? Reset the counters in this case.
        check(lastExecutedThread == iThread) { "reset expected!" }
        // Ignore coroutine suspension code locations.
-        if (codeLocation == COROUTINE_SUSPENSION_CODE_LOCATION) return Decision.Idle


Why UNKNOWN? The documentation here and of the UNKNOWN_CODE_LOCATION field says it is the coroutine suspension code location.

eupp · 2024-12-12T12:01:31Z

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

dynamic-threads branch
develop branch

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:

lincheck/bootstrap/src/sun/nio/ch/lincheck/Injections.java

Line 91 in 3feb8a9

if (thread instanceof TestThread) {

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

eupp force-pushed the dynamic-threads branch from 2f30c84 to 9fdd0e4 Compare October 11, 2024 21:34

eupp marked this pull request as ready for review October 12, 2024 01:30

eupp requested a review from ndkoval October 12, 2024 01:30

ndkoval reviewed Oct 15, 2024

View reviewed changes

...vm/test/org/jetbrains/kotlinx/lincheck_test/transformation/KotlinStdlibTransformationTest.kt Outdated Show resolved Hide resolved

eupp force-pushed the dynamic-threads branch 2 times, most recently from 0ad50bd to 25c0c45 Compare October 31, 2024 17:15

eupp requested a review from ndkoval October 31, 2024 17:20

eupp force-pushed the dynamic-threads branch from 25c0c45 to bac6712 Compare October 31, 2024 17:37

eupp mentioned this pull request Nov 7, 2024

Fix local objects detector #422

Merged

eupp force-pushed the dynamic-threads branch from bac6712 to 250236c Compare November 26, 2024 10:55

eupp force-pushed the dynamic-threads branch from 8d548c9 to 9a4fdcd Compare November 27, 2024 14:55

eupp added 16 commits December 11, 2024 16:22

move beforePart logic from ModelCheckingStrategy to `ManagedStrat…

321d664

…egy` Signed-off-by: Evgeniy Moiseenko <[email protected]>

rename onStart/onFinish methods to onThreadStart/onThreadFinish

f67e943

Signed-off-by: Evgeniy Moiseenko <[email protected]>

refactor ManagedStrategy to use ThreadScheduler class

7ced1d1

Signed-off-by: Evgeniy Moiseenko <[email protected]>

refactor ManagedStrategy to use ThreadScheduler class for threads…

777195c

… blocking Signed-off-by: Evgeniy Moiseenko <[email protected]>

add TODOs about coroutine suspensions

b39cfed

Signed-off-by: Evgeniy Moiseenko <[email protected]>

rework Spinner class so it can work with a dynamically changing thr…

2498db1

…ead group Signed-off-by: Evgeniy Moiseenko <[email protected]>

rename ForcibleExecutionFinishError into ThreadAbortedError

945369f

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor refactoring of ignored sections

da858c2

* in preparation of implementing ignored sections for custom threads Signed-off-by: Evgeniy Moiseenko <[email protected]>

document ignored section methods in Injections.java

16c26be

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add ignored sections methods to EventTracker, unifying the logic of…

0de2820

… ignored sections Signed-off-by: Evgeniy Moiseenko <[email protected]>

implement global event tracker, consolidating ignored sections logic

13e023a

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor renaming

dd91297

Signed-off-by: Evgeniy Moiseenko <[email protected]>

consolidating ignored sections logic: use runOutsideIgnoredSection …

815fd39

…in `cancelByLincheck` Signed-off-by: Evgeniy Moiseenko <[email protected]>

consolidating testing code flag logic

d321706

Signed-off-by: Evgeniy Moiseenko <[email protected]>

moving testing code and ignored section flags into thread scheduler c…

8bdba76

…lass Signed-off-by: Evgeniy Moiseenko <[email protected]>

moving testing code and ignored section flags into thread scheduler c…

d95725f

…lass Signed-off-by: Evgeniy Moiseenko <[email protected]>

eupp added 25 commits December 11, 2024 16:24

Revert "try to leave testing code once ThreadAbortedError is thrown"

c45acfa

This reverts commit 2d52b3a.

fix for Java 8 (remove usages of newer Java features)

17cb610

Signed-off-by: Evgeniy Moiseenko <[email protected]>

fix for Java 8 (representation tests)

fcc7432

Signed-off-by: Evgeniy Moiseenko <[email protected]>

increase number of invocations in ExceptionTest to prevent flakiness

f2d196c

Signed-off-by: Evgeniy Moiseenko <[email protected]>

handle potential problems with hash code collisions and memory leaks …

0730c20

…in the thread descriptors map Signed-off-by: Evgeniy Moiseenko <[email protected]>

properly handle weak references in ThreadDescriptor class

83f2b4b

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add custom threads representation test

a07a222

Signed-off-by: Evgeniy Moiseenko <[email protected]>

make TraceCollector to work with custom threads

b096ce1

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor in CustomThreadsRepresentationTest

e156a37

Signed-off-by: Evgeniy Moiseenko <[email protected]>

update CustomThreadsRepresentationTest expected output

26e5144

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor in CustomThreadsRepresentationTest

6a59af4

Signed-off-by: Evgeniy Moiseenko <[email protected]>

suppress Unsafe deprecation

803c5eb

Signed-off-by: Evgeniy Moiseenko <[email protected]>

fix minor capacity bug

0f9fe11

Signed-off-by: Evgeniy Moiseenko <[email protected]>

fix Java 8 trace for CustomThreadsRepresentationTest

a21d100

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add trace join trace point

b620a18

Signed-off-by: Evgeniy Moiseenko <[email protected]>

fix basic custom threads trace logs (add thread join events)

a2d6fde

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor

56b5577

Signed-off-by: Evgeniy Moiseenko <[email protected]>

exit early in beforeMethodCall in case of abort

6916dab

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add custom threads spin-lock test

009bd98

Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor fix: in beforeMethodCall re-throw ThreadAborted exception i…

a0cee55

…n case of abort Signed-off-by: Evgeniy Moiseenko <[email protected]>

minor

6411058

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add THREAD_START trace point

9678e73

Signed-off-by: Evgeniy Moiseenko <[email protected]>

use Java Thread API in custom threads representation tests to simplif…

7f379e9

…y the output Signed-off-by: Evgeniy Moiseenko <[email protected]>

add Kotlin Thread API threads representation test

7a743c2

Signed-off-by: Evgeniy Moiseenko <[email protected]>

add custom threads data structures representation tests

469be4f

Signed-off-by: Evgeniy Moiseenko <[email protected]>

eupp force-pushed the dynamic-threads branch from 2428b4f to 469be4f Compare December 11, 2024 15:26

eupp added 2 commits December 11, 2024 17:36

fix representation tests (remove sun.nio.ch.lincheck.TestThread fro…

78b4428

…m stack traces) Signed-off-by: Evgeniy Moiseenko <[email protected]>

fix representation tests (Java 8 trace)

3feb8a9

Signed-off-by: Evgeniy Moiseenko <[email protected]>

ndkoval requested changes Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom threads #415

Support custom threads #415

eupp commented Oct 11, 2024

eupp commented Oct 12, 2024

eupp commented Oct 31, 2024

eupp commented Nov 26, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

eupp commented Dec 12, 2024 •

edited

Loading

Support custom threads #415

Are you sure you want to change the base?

Support custom threads #415

Conversation

eupp commented Oct 11, 2024

eupp commented Oct 12, 2024

eupp commented Oct 31, 2024

eupp commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eupp commented Dec 12, 2024 • edited Loading

eupp commented Dec 12, 2024 •

edited

Loading