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

Initial Ember Port #2696

Merged
merged 13 commits into from Jul 9, 2019
Merged

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jul 5, 2019

No description provided.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Jul 5, 2019

🎉

Loading

Copy link
Member

@rossabaker rossabaker left a comment

Some of these comments are useful. Some are an old man ranting. But I'm thrilled to see this and would rather get it in sooner than later so we can get more eyes on it and continue to harden it. Great work.

Loading

Loading
Loading
Loading
Loading
def withChunkSize(chunkSize: Int) = copy(chunkSize = chunkSize)
def withMaxResponseHeaderSize(maxResponseHeaderSize: Int) =
copy(maxResponseHeaderSize = maxResponseHeaderSize)
def withTimeout(timeout: Duration) = copy(timeout = timeout)
Copy link
Member

@rossabaker rossabaker Jul 6, 2019

Choose a reason for hiding this comment

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

What is this timeout?

Loading

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Jul 8, 2019

Choose a reason for hiding this comment

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

I believe a full message read baseline. For if it has taken too long. See ember.core.Util.readWithTimeout

Loading

Loading
Loading
Loading
Loading
Loading
Copy link
Contributor

@diesalbla diesalbla left a comment

Nice. Just a few comments.

Loading

Loading
Loading
Loading
.guard(req.isChunked)
.fold(req.body)(_ => req.body.through(ChunkedEncoding.encode[F]))

initSection.covary[F].intersperse("\r\n").through(text.utf8Encode) ++
Copy link
Contributor

@diesalbla diesalbla Jul 7, 2019

Choose a reason for hiding this comment

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

I have a question about this encoding/decoding.

Now, the goal of this encoding is to generate a stream of bytes (a sequence of vectors of bytes) that are to be sent through the network, which involves an I/O operation and an Operating System call, which may be costly. If so, one may seek to reduce number of I/O calls, by buffering as many of those as possible. In this context, would it make sense to "aggregate" as many of these strings or byte-vectors as possible into a single ByteVector page, which may be sent to the O/S in a single step?

At present, I am not sure if each element in the stream would involve a different "I/O" operation and OS call.

Loading

Copy link
Member

@rossabaker rossabaker Jul 8, 2019

Choose a reason for hiding this comment

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

The fastest thing is to build one String for the entire header and encode it in one pass. This outperforms fs2 Pipes and NIO's incremental charset encoder (which I believe the pipes use).

Loading

Copy link
Contributor

@diesalbla diesalbla Jul 8, 2019

Choose a reason for hiding this comment

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

A recommended way to build a long string, made of concatenating many small strings, is by using a StringBuilder. Is there also a single-pass encoder for that? Or a ByteVector-builder?

Loading

Copy link
Member

@rossabaker rossabaker Jul 8, 2019

Choose a reason for hiding this comment

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

Encoding incrementally to ByteVector is slow. I've created ByteVectorWriter a few times and thrown it away each time because it never wins.

There's no way that I'm aware of to encode directly from a StringBuilder, though toString() is cheap.

Loading

Loading
Loading
.guard(req.isChunked)
.fold(req.body)(_ => req.body.through(ChunkedEncoding.encode[F]))

initSection.covary[F].intersperse("\r\n").through(text.utf8Encode) ++
Copy link
Member

@rossabaker rossabaker Jul 8, 2019

Choose a reason for hiding this comment

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

The fastest thing is to build one String for the entire header and encode it in one pass. This outperforms fs2 Pipes and NIO's incremental charset encoder (which I believe the pipes use).

Loading

Copy link
Member

@rossabaker rossabaker left a comment

I restarted the flaky build. There are a couple new bikesheds to paint, but I think this is in good enough shape now, and further improvement will be easier in smaller PRs.

Loading

@ChristopherDavenport ChristopherDavenport merged commit 8a37adc into http4s:master Jul 9, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants