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

added: fileSourceByteString #1503

Merged
merged 10 commits into from
May 4, 2018
Merged

Conversation

ncaq
Copy link
Contributor

@ncaq ncaq commented Apr 17, 2018

This function is to get FileInfo raw body.

Conduit type is hard for begginer user.

Actually I was not sure when I was a Haskell beginner.

ByteString raw body of FileInfo is need to upload file to S3.
This function running our product.
I send it to upstream.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

This function is to get `FileInfo` raw body.
--
-- @since 1.6.4
fileSourceByteString :: MonadResource m => FileInfo -> m S.ByteString
fileSourceByteString fileInfo = fileSource fileInfo `connect` CL.foldMap id
Copy link
Member

Choose a reason for hiding this comment

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

There are a few problems with this implementation:

  1. CL.foldMap id has quadratic complexity. It would be better to use something like sinkLazy
  2. It's more common to use runConduit $ src .| sink

How about:

runConduit $ fileSource fileInfo .| Conduit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use runConduit.
But fileSource function result strict ByteString.
So I think can't use sinkLazy.
Do you think?

Copy link
Member

Choose a reason for hiding this comment

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

You'd have to use Data.ByteString.toStrict, but that's more efficient than using foldMap.

@@ -1360,6 +1361,16 @@ rawRequestBody = do
fileSource :: MonadResource m => FileInfo -> ConduitT () S.ByteString m ()
fileSource = transPipe liftResourceT . fileSourceRaw

-- | Strict `ByteString` body from `FileInfo`.
-- This function blocking while read file.
Copy link
Member

Choose a reason for hiding this comment

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

Can you reword this comment to something like:

Extract a strict `ByteString` body from a `FileInfo`.

This function will block while reading the file.

@ncaq
Copy link
Contributor Author

ncaq commented Apr 17, 2018

@snoyberg I think that sinkLazy can't use this case.
Do you think?

@ncaq
Copy link
Contributor Author

ncaq commented Apr 17, 2018

@snoyberg
I use sinkLazy.
However, Maybe fileSourceByteString return Lazy ByteString is great than Strict?

@snoyberg
Copy link
Member

Returning a lazy ByteString instead is a good idea, though then it should be documented to clarify that the value will still be strictly read (as opposed to lazy I/O).

@ncaq
Copy link
Contributor Author

ncaq commented Apr 24, 2018

@snoyberg
I think again.
fileSource is strict, so fileSourceByteString should be strict.

So, I use foldC. Do you think about?

@snoyberg
Copy link
Member

foldC has the same quadratic behavior issues as foldMapC.

@ncaq
Copy link
Contributor Author

ncaq commented Apr 24, 2018

@snoyberg
Sorry, What are "complexity" mean code clearing or performance?
I understood to code clearing.
So, I try to write clear code.
But, Are you speak performance?

@snoyberg
Copy link
Member

snoyberg commented Apr 24, 2018 via email

Because performance problem.
@ncaq
Copy link
Contributor Author

ncaq commented May 1, 2018

@snoyberg I rewrite.

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Thanks!

@snoyberg snoyberg merged commit 5861357 into yesodweb:master May 4, 2018
@ncaq ncaq deleted the add-file-source-bytes branch May 4, 2018 09:31
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