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

updating hyper to 0.12.16 and increasing http request buffer size to 2MB #1227

Merged
merged 4 commits into from Dec 8, 2018

Conversation

Projects
None yet
4 participants
@rrichardson
Copy link
Contributor

rrichardson commented Nov 26, 2018

Updated hyper dependency to v0.12.16 and increased the http request buffer size to 2MB.

@rrichardson

This comment has been minimized.

Copy link
Contributor

rrichardson commented Nov 26, 2018

This addresses #1226

@srijs

This comment has been minimized.

Copy link
Member

srijs commented Nov 26, 2018

Thanks!

Bumping from 8KB to 2MB is quite a bump, and while I appreciate that it helps performance in your object store benchmarks, I can't help but wonder if it's the right trade-off for smaller JSON/XML messages. Have you thought about making the limit configurable as it is in hyper?

@rrichardson

This comment has been minimized.

Copy link
Contributor

rrichardson commented Nov 26, 2018

I think making it configurable is definitely the correct answer. I didn't really spot a config/builder scheme for the core http client. If you can point me to one that'd be great. If you don't have such a thing, what would be your recommendation for where to inject the API?

@@ -26,7 +26,7 @@ rustc_version = "0.2.1"
[dependencies]
futures = "0.1.16"
hmac = "0.5.0"
hyper = "0.12.0"
hyper = "0.12.16"

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 27, 2018

Member

I believe the existing 0.12.0 specification will pick up the latest version, 0.12.16, when it's being built. Does this change help that by not requiring Rusoto users to delete their Cargo.lock file or run cargo update?

This comment has been minimized.

@rrichardson

rrichardson Nov 27, 2018

Contributor

Does 0.12.0 do it, or would it need to be 0.12?

I interpreted 0.12.0 as specifying exactly 0.12.0 which is why I specified 0.12.16

This comment has been minimized.

@matthewkmayer

matthewkmayer Nov 27, 2018

Member

This often trips me up as well. 😄 The latest cargo guide on specifying dependencies has improved in clarity:

[dependencies]
time = "0.1.12"

The string "0.1.12" is a semver version requirement. Since this string does not have any operators in it, it is interpreted the same way as if we had specified "^0.1.12", which is called a caret requirement.
...
Caret requirements allow SemVer compatible updates to a specified version. An update is allowed if the new version number does not modify the left-most non-zero digit in the major, minor, patch grouping. In this case, if we ran cargo update -p time, cargo should update us to version 0.1.13 if it is the latest 0.1.z release, but would not update us to 0.2.0.

If we really wanted hyper 0.12.16 and only 0.12.16, we could specify it like this:

hyper = "=0.12.16"

Hope this makes sense! 😄

This comment has been minimized.

@softprops

softprops Nov 29, 2018

Contributor

Wow I did not know that. TIL!

This comment has been minimized.

@rrichardson

rrichardson Nov 29, 2018

Contributor

I changed it to

hyper = "0.12"

to be more clear that we're defaulting to the latest patch version of 0.12

@softprops

This comment has been minimized.

Copy link
Contributor

softprops commented Nov 27, 2018

+1 for configurability of this means my applications are going to start consuming that much more memory

rrichardson added some commits Nov 29, 2018

@rrichardson

This comment has been minimized.

Copy link
Contributor

rrichardson commented Nov 29, 2018

I've created an HttpConfig struct which is supplied to HttpClient::new_with_config.
I couldn't find an unobtrusive way to alter the Client::shared() function, so basically the HttpConfig feature is only available to people who construct the Client with their own Dispatcher and CredentialsProvider

@rrichardson

This comment has been minimized.

Copy link
Contributor

rrichardson commented Nov 29, 2018

I rebuilt my object-spam s3 benchmarker against this PR branch using 8MB buffers.
The CPU usage dropped from 300% on 3 threads down to 80-100% on 3 threads.
The overall read bandwidth went up 30% as well.

So the fix definitely works :)

@matthewkmayer matthewkmayer added this to the 0.36.0 milestone Dec 1, 2018

@matthewkmayer

This comment has been minimized.

Copy link
Member

matthewkmayer commented Dec 1, 2018

I should get to a review in the next day or two, I'm really excited about getting this change in. ❤️

This PR will be one of the last ones in before our 0.36 release. 🎉

/// with extra configuration options
pub fn from_connector_with_config(connector: C, config: HttpConfig) -> Self {
let mut builder = HyperClient::builder();
config.read_buf_size.map(|sz| builder.http1_read_buf_exact_size(sz));

This comment has been minimized.

@matthewkmayer

matthewkmayer Dec 1, 2018

Member

I'm not certain this works as expected: looks like we're running map on the results of the read_buf_size function, which is the unit type of (). I think to get the value we want from HttpConfig we'll need to either make the struct's read_buf_size field publicly accessible or make a getter function and use it here.

Since testing shows this works, I may be missing something. Would love it pointed out. 😄

This comment has been minimized.

@matthewkmayer

This comment has been minimized.

Copy link
Member

matthewkmayer commented Dec 1, 2018

My strawman PR of #1228 has an integration test using the new behavior. The only reason I picked ECS was it was a small service that compiles fast. Perhaps there's a better place to demonstrate this new behavior, but I'm not certain where that would be. I really want an example to live in the repo somewhere, though.

@matthewkmayer

This comment has been minimized.

Copy link
Member

matthewkmayer commented Dec 2, 2018

How about this: unless we find a great place to put the sample using this new feature, we use the ECS integration test for now, like #1228 . We can always move things around if/when we find a better spot.

Once we've got that done this PR is good to go!

@matthewkmayer

This comment has been minimized.

Copy link
Member

matthewkmayer commented Dec 5, 2018

Let me know if you'd like a hand putting the integration test into this PR, showing off this new feature. This should be the last PR before our next release. 🎉

@matthewkmayer matthewkmayer referenced this pull request Dec 8, 2018

Merged

Release 0.36.0 #1234

@matthewkmayer matthewkmayer merged commit fd861f4 into rusoto:master Dec 8, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment