Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
optimize ZStream's mapZIOPar and mapZIOParUnordered #8819
optimize ZStream's mapZIOPar and mapZIOParUnordered #8819
Changes from 28 commits
411f287
6efc2d7
d911350
59533ad
277bf35
58cabb4
0c31b9d
b7cb92e
6596142
560ebcd
b9eb3ac
42dfe12
6ad59d8
7ad415b
fe66a4d
1e2bfd8
8118984
077898f
5bbfe03
b228693
e8cc44f
d6c9793
6cf8265
918aa6c
11e09e1
08d6510
a7246ad
552a6b4
6f81f82
9fd7ff3
3bdea62
d921095
eb1ef45
ba2e2ef
b8d2dbd
6edd498
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this optimization will cause
.children
to block if the underlying fiber is blocked. This means that operations like fiber dump will not work properly on Loom.I would like to eliminate the synchronization on the children set, too, but I'm afraid it is probably going to require a new lock-free set implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u please elaborate on this? under the current implementation the fiber is 'waking up' on queued messages, what's going to happen in loom? r u concerned about scenarios where the fiber is blocking on something like promises? does this mean
FiberMessage.Stateful
is going away? will FiberRuntime even have a queue like it does today?furthermore, current implementation relies on
FiberMessage.Stateful
for common operations liketellAddChild
(used byzio.internal.FiberScope.Local#add
), so I don't think this specific change introduces any new blocking risk.seems like going 'loom' will require substantial changes to zio fibers...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a big optimization potential here. Currently we're initializing the Set using the default underlying size (size 16, can accommodate 12 entries before resizing).
Since now we can add children in bulk, we therefore know how many entries the set is meant to hold (at least initially). So we can instantiate it with a size large enough to accommodate all the entries without the need to dynamically resize.
Note that to calculate the size based on the number of expected entries you need to use this equation:
size = Math.ceil(nEntries / 0.75d)
I would also advice to use 16 as the minimum size as we don't get much benefit sizing it smaller, but can be really bad if we need to keep resizing later (since resizing happens in Pow2 increments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought about this potential, I think it goes beyond that since some of the spawned fibers ae very short lived and may already e finished by the time they're added to the fiber scope.
another thought I had, when forking multiple 'very similar' fibers (foreachParXXX), is it possible to come up with some kind of 'template' for the inherited fiber refs? they are all forked from the same parent so all should have very similar fiber refs except from the few which have a specialized 'fork' logic which perhaps can be pre-identified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, is it mandatory for
_children
to be a set? theremove
operation is currently not used, we also never add the same fiber twice so the collection is effectively guaranteed to be a set.I suspect an array based impl with 'occasional' GC can be sufficient here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyri-petrou I suspect I may have to restore the synchronized weak set, seems like the set's
isEmpty
method effectively modifies it.if there was a way to get the 'naive' set's size we'd still be ok, but given the current situation I think we must restore the synchronized weak set and seek alternative (I still think this doesn't have to be a set at all).
another alternative is to restore the message based
children
impl and address @jdegoes comment once ZIO actually goes loom.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks