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

Add ActionRegistry replacement for TempRegistry #484

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

jorisdral
Copy link
Collaborator

@jorisdral jorisdral commented Dec 2, 2024

I've reworked the TempRegistry implementation a bit, and I've renamed it to ActionRegistry to better reflect that it is a registry of monadic actions. The ActionRegistry is introduced in a new module so that the TempRegistry can largely remain in use, which makes the diff of this PR smaller. The intent is to replace every use of TempRegistry by ActionRegistry eventually, probably in a few follow-up PRs. For now, there are two places where we do the replacement already: openSession and closeSession. I've also taken the liberty to revisit openSession and closeSession a bit to make them more exception safe

@jorisdral jorisdral self-assigned this Dec 2, 2024
@jorisdral jorisdral force-pushed the jdral/action-registry branch from ee61973 to 4888580 Compare December 2, 2024 16:07
src-control/Control/ActionRegistry.hs Outdated Show resolved Hide resolved
src/Database/LSMTree/Internal.hs Outdated Show resolved Hide resolved
@jorisdral jorisdral marked this pull request as ready for review December 2, 2024 16:10
Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

So just trying to summarise the changes vs TempRegistry:

  • better naming (commit / release of actions rather than temp and frees resources)
  • no support for sharing a registry between threads, but hence also cheaper (IORefs rather than MVars)
  • generally cheaper: simple LIFO lists rather than maps

We need SPECIALISE pragmas everywhere in the ActionRegistry.hs.

It looks like ActionRegistry would be a good place to insert tests for exception safety. We could use fs-sim style exception testing to throw exceptions at random points within a transaction (when actions are registered) and verify that everything gets cleaned up properly.

src-control/Control/ActionRegistry.hs Outdated Show resolved Hide resolved
src-control/Control/ActionRegistry.hs Outdated Show resolved Hide resolved
src-control/Control/ActionRegistry.hs Show resolved Hide resolved
src-control/Control/ActionRegistry.hs Show resolved Hide resolved
src/Database/LSMTree/Internal.hs Outdated Show resolved Hide resolved
@jorisdral jorisdral force-pushed the jdral/action-registry branch from 4888580 to c7d19f4 Compare December 18, 2024 13:09
@jorisdral
Copy link
Collaborator Author

So just trying to summarise the changes vs TempRegistry:

* better naming (commit / release of actions rather than temp and frees resources)

* no support for sharing a registry between threads, but hence also cheaper (IORefs rather than MVars)

* generally cheaper: simple LIFO lists rather than maps

Yes, and other changes I can think of:

  • runActions tries to run actions even if some of them fail with an exception

  • More coherent documentation (I hope)

We need SPECIALISE pragmas everywhere in the ActionRegistry.hs.

Yes, good point. Added them.

It looks like ActionRegistry would be a good place to insert tests for exception safety. We could use fs-sim style exception testing to throw exceptions at random points within a transaction (when actions are registered) and verify that everything gets cleaned up properly.

I have some uncommitted tests for this using IOSimPOR, but it's tricky to get right and I just need to invest more time into it. Using fs-sim to throw errors also works to some extent, but you don't get as precise an interleaving of operations as in IOSim/ IOSimPOR

@jorisdral jorisdral force-pushed the jdral/action-registry branch from cc52e46 to 9934b2d Compare December 18, 2024 13:59
Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM. Squash fixups and merge I say.

@jorisdral jorisdral force-pushed the jdral/action-registry branch from 738e376 to 2f396e9 Compare December 18, 2024 16:32
@jorisdral jorisdral enabled auto-merge December 18, 2024 16:32
@jorisdral jorisdral added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit 42711c5 Dec 18, 2024
27 checks passed
@jorisdral jorisdral deleted the jdral/action-registry branch December 18, 2024 17:38
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