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

Spin locks detection enchancements #331

Merged
merged 15 commits into from
Jul 10, 2024
Merged

Conversation

avpotapov00
Copy link
Collaborator

Closes #218, #219, adds recursive spin locks support and overall spin locks detection enhancements.

@avpotapov00 avpotapov00 requested a review from ndkoval June 10, 2024 00:19
@avpotapov00 avpotapov00 self-assigned this Jun 10, 2024
@avpotapov00 avpotapov00 changed the base branch from master to develop June 10, 2024 00:19
Aleksandr.Potapov added 4 commits June 10, 2024 10:19
# Conflicts:
#	bootstrap/src/sun/nio/ch/lincheck/EventTracker.kt
#	src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt
#	src/jvm/main/org/jetbrains/kotlinx/lincheck/transformation/LincheckClassVisitor.kt
#	src/jvm/test/org/jetbrains/kotlinx/lincheck_test/AbstractLincheckTest.kt
@avpotapov00 avpotapov00 requested a review from eupp June 10, 2024 13:39
@avpotapov00 avpotapov00 requested a review from eupp June 16, 2024 20:07
@avpotapov00 avpotapov00 requested a review from eupp June 22, 2024 14:16
@ndkoval
Copy link
Collaborator

ndkoval commented Jun 26, 2024

@avpotapov00, could you please check the documentation with Grammarly or Grazie?

* Provides the prefix output for the [TraceNode].
* @see TraceNodePrefixFactory
*/
internal fun interface PrefixProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it always the same string for each instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not the same string, prefix reflects depth call and sometimes includes arrows parts of the spin cycle. See the implementation and usages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you create a new PrefixProvider, does it always generate the same prefix? Or how does it work? What does Provider mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please take a look at the org.jetbrains.kotlinx.lincheck.strategy.managed.TraceNodePrefixFactory documentation: it is described there.

@@ -821,6 +828,7 @@ abstract class ManagedStrategy(
null
}
newSwitchPoint(iThread, codeLocation, tracePoint)
loopDetector.beforeWriteArrayElement(array, index, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be called before newSwitchPoint or after? Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current implementation it doesn't matter. But it looks more logical after the newSwitchPoint call as after we back to a thread we want to trace field read after the switch, not before (as it will actually happen).

@@ -797,6 +803,7 @@ abstract class ManagedStrategy(
null
}
newSwitchPoint(iThread, codeLocation, tracePoint)
loopDetector.beforeWriteField(obj, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be called before newSwitchPoint or after? Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current implementation it doesn't matter. But it looks more logical after the newSwitchPoint call as after we back to a thread we want to trace field read after the switch, not before (as it will actually happen).

@ndkoval
Copy link
Collaborator

ndkoval commented Jun 27, 2024

General comment: would the current IDEA plugin work with this PR being merged?

Aleksandr.Potapov and others added 2 commits July 2, 2024 18:35
# Conflicts:
#	bootstrap/src/sun/nio/ch/lincheck/EventTracker.kt
#	src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt
#	src/jvm/test/resources/expected_logs/spin_lock_events_cut_inner_loop.txt
@avpotapov00 avpotapov00 requested a review from ndkoval July 3, 2024 15:15
@ndkoval ndkoval requested a review from eupp July 8, 2024 18:31
Copy link
Collaborator

@ndkoval ndkoval left a comment

Choose a reason for hiding this comment

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

@eupp, please review the recent changes and merge the PR when it's ready.

@ndkoval ndkoval mentioned this pull request Jul 9, 2024
2 tasks
@avpotapov00 avpotapov00 merged commit 26f2bb6 into develop Jul 10, 2024
15 checks passed
@avpotapov00 avpotapov00 deleted the recusrive-spin-locks branch July 10, 2024 14:23
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.

Spin cycle start label placed incorrectly when first cycle operation is some non-atomic method call
3 participants