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

newFastLogger1 ensures the ordering of logs #207

Merged
merged 12 commits into from Feb 9, 2023
Merged

Conversation

kazu-yamamoto
Copy link
Owner

@carbolymer @khibino This should fix #203.

The old logic cannot ensure the ordering of logs even if the number of internal builders are 1.
This PR calls the old one MultiLogger and introduces a new one called SingleLogger.
SingleLogger uses a queue of builder in addition to an internal builder.
The internal builder gets empty and the queue glows atomically.
The builders of the queue are written by a single dedicated thread, thus the ordering is maintained.
A test case is also provided.

Copy link

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Your solution looks sound to me. Thanks for that fix!

writer buffd@BufFD{..} tvar ref mvar = loop (0 :: Int)
where
loop cnt = do
cnt' <- atomically $ do
Copy link

@carbolymer carbolymer Feb 8, 2023

Choose a reason for hiding this comment

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

I'm wondering if you're already using TVar for spin-waiting, wouldn't it be easier to just use TQueue and a poison pill instead of TVar, IORef and get rid of MVar for signalling shutdown?

P.S. It took me a while to decipher the meaning of tvar , ref, mvar. Maybe it would be a good idea to use more meaningful names?

Copy link
Owner Author

Choose a reason for hiding this comment

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

STM is heavier than atomicModifyIORef'.
In this design, STM transactions run only when the builder reaches the limit.

Yes, MVar Buffer is not necessary. I should think whether or not we can remove it from SingleLogger easily.

Copy link

@khibino khibino left a comment

Choose a reason for hiding this comment

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

I looked at SingleLogger and it looks good.

@kazu-yamamoto
Copy link
Owner Author

@carbolymer @khibino Now SingleLogger does not use MVar to protect Buffer since he is a single user of the Buffer. Please check it out again.

Copy link

@khibino khibino left a comment

Choose a reason for hiding this comment

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

LGTM

@kazu-yamamoto kazu-yamamoto merged commit e145493 into master Feb 9, 2023
@kazu-yamamoto kazu-yamamoto deleted the one-writer branch February 9, 2023 03:38
@kazu-yamamoto
Copy link
Owner Author

Merged. I will release a new major version!

@kazu-yamamoto
Copy link
Owner Author

Released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants