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

#12 #48 WcApp implementation using ZStream #64

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

TobiasPfeifer
Copy link
Contributor

@TobiasPfeifer TobiasPfeifer commented Nov 21, 2020

#12 #48

@jdegoes @iravid for files larger than 1MB this implementation is consuming 100% CPU for way to long. Can you check if there is a flaw in my implementation. Otherwise this is indicating a performance issue in ZStreams

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2020

CLA assistant check
All committers have signed the CLA.

wordsStream <- if (opts.words) words.optional else ZIO.none
linesStream <- if (opts.lines) lines.optional else ZIO.none

bytesFiber <- runCountOrNone(bytesStream).fork
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately you will read the file 4 times. I think you want 4 transducers, each of them doing word / character / line / byte count, and then glue them together, and stick them on the stream. That would be much more efficient than reading the file 4 separate times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do ZTransducer compose in that way? There is not zip on ZTransducers. But I get your point there. I'll implement it using ZSinks. One for each optional count and then zipping them and apply it on the stream.

Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate they don't compose (although they can). So yes, I think switching to using ZSink is a good idea.

@jdegoes jdegoes merged commit 09c8f2e into zio:master Nov 24, 2020
@jdegoes
Copy link
Member

jdegoes commented Nov 24, 2020

@TobiasPfeifer Great work! So nice to see a full-fledged and working example app written using ZIO CLI.

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.

3 participants