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

Internal trackers APIs #369

Merged
merged 10 commits into from
Oct 1, 2024
Merged

Internal trackers APIs #369

merged 10 commits into from
Oct 1, 2024

Conversation

eupp
Copy link
Collaborator

@eupp eupp commented Aug 29, 2024

This PR implements a refactoring to introduce separate internal event trackers API:

  • MonitorTracker tracks monitor events (lock, unlock, wait, notify)
  • ParkingTracker tracks parking events (park, unpark)
  • ObjectTracker tracks new objects creation and changes in object graph topology.

Different strategies can implement these tracker interfaces for their needs.

We need this refactoring for two features:

  • new model checking strategy need to implement its own logic for tracking aforementioned events;
  • to support blocking operations, stress strategy also need to track parking events.

@eupp eupp marked this pull request as draft August 29, 2024 13:38
@eupp
Copy link
Collaborator Author

eupp commented Aug 29, 2024

@avpotapov00 could you please have a look at this PR and check if it aligns with your vision for the parking tracker for stress strategy.

@eupp eupp requested a review from avpotapov00 August 29, 2024 13:39
@eupp eupp marked this pull request as ready for review September 25, 2024 14:42
@eupp eupp requested a review from ndkoval September 26, 2024 16:11
@eupp
Copy link
Collaborator Author

eupp commented Sep 26, 2024

@ndkoval ready for review.
We also discussed it with @avpotapov00 , he is also good with these changes.

@ndkoval
Copy link
Collaborator

ndkoval commented Sep 26, 2024

@eupp, great, I will take a look at the PR. Meanwhile, could you please investigate why the build on Windows has failed?

@eupp
Copy link
Collaborator Author

eupp commented Sep 26, 2024

Meanwhile, could you please investigate why the build on Windows has failed?

Looks like unrelated flakiness problem. I've re-run the build and it is green now.

*
* @param threadId the id of the thread to check for parking status.
* @return true if the thread should continue waiting,
* false if it was already unparked.
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 return false if the execution has finished?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By "execution has finished" do you mean "all other threads finished"?

At least in current implementation, ParkingTracker does not have access to threads "finish status" info. Parking tracker only stores parked boolean flags for each thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So can I merge this PR? @ndkoval

@ndkoval
Copy link
Collaborator

ndkoval commented Sep 30, 2024

Hi @eupp, I've approved the PR. Thanks for extracting the logic and improving the code structure!

I've left a minor comment -- please address it and merge the PR.

eupp added 10 commits October 1, 2024 15:16
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]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp eupp merged commit 621de9e into develop Oct 1, 2024
15 checks passed
@eupp eupp deleted the trackers-api branch October 1, 2024 17:14
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