Closed roman closed 5 years ago
Hey @snoyberg, let me know if the changes in this Pull Request is acceptable for our purposes. Some of the stuff I did:
Alternative
instance of Conc
)TMVar
, but wasn't able to get anything acceptable off the ground.Also, I spent a good chunk of the time trying to make the PR pass the CI tests, so that's why you will see most of the commits being dedicated to that.
@roman I think it would be a good idea to adapt tests from this PR here also: https://github.com/simonmar/async/pull/21/files
@psibi that's a great idea, I'll try to port some of those tests for our Conc
type
@snoyberg I did a review with @nh2, and he was happy with most of the code, I went ahead and added a Lift
constructor in addition to LiftA2
and benchmarked improved marginally. Let me know your thoughts.
@snoyberg I think all the comments have been resolved, and CI is green, can you do one more check, and merge if you see if everything is good 🙏? Thank you
Ensure this work gets merged.
Investigation notes:
Investigate any remaining performance enhancements (such as using less locking and more IORefs)
I tried to remove the usage of
TMVar
from the innergo
function in the outerrunFlat
function. This seemed rather convoluted as then the composition of pending jobs on theFlatLiftA2
becomes particularly cumbersome.If the current reader has any thoughts in regards to having a strategy that drops
TMVar
in favor of a collection inside anIORef
and usingatomicModifyIORef
, I'm interested :-).