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

feat(proto): update to time 0.2 #2139

Closed
wants to merge 1 commit into from
Closed

feat(proto): update to time 0.2 #2139

wants to merge 1 commit into from

Conversation

koushiro
Copy link

No description provided.

@seanmonstar
Copy link
Member

Thanks for the PR!

I wonder, how "weighty" is time v0.2? We really only need the ability to format a SystemTime to a single specific format...

Copy link

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

I was looking to upgrade some more reverse dependencies, and hyper is among the most commonly used of them. I see this PR is open, so I may as well review it!

Sean — the overhead compared to 0.1 for this specific use case shouldn't be much, if any at all. I'm not familiar with hyper's internals, so I likely wouldn't be able to create an accurate benchmark. I think the only difference would be that this calculates the year, month, day, hour, minute, and second up-front as opposed to at formatting time. This does add a slight overhead in memory usage (56 bytes to 64) due to the presence of the stored UTC offset & additional padding. If you're careful, this could be worked around, which would bring the size back down to 56 bytes per my quick check. I'd only do that if it has noticeable performance improvements, though.

@@ -52,7 +52,7 @@ serde_json = "1.0"
tokio = { version = "0.2.2", features = ["fs", "macros", "io-std", "rt-util", "sync", "time", "test-util"] }
tokio-test = "0.2"
tower-util = "0.3"
url = "1.0"
url = "2.0"
Copy link

Choose a reason for hiding this comment

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

I'm by no means the maintainer of hyper, but I would imagine keeping this separate would be best.

Comment on lines +65 to +71
let nanosecond = self.next_update.time().nanosecond();
self.next_update = now + Duration::seconds(1) - Duration::nanoseconds(nanosecond as i64);
Copy link

Choose a reason for hiding this comment

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

Easier to just add the number of nanoseconds directly. It may be worth checking the optimized output of this, though, as the compiler may be able to optimize away the x - x.

Suggested change
let nanosecond = self.next_update.time().nanosecond();
self.next_update = now + Duration::seconds(1) - Duration::nanoseconds(nanosecond as i64);
self.next_update = now + Duration::nanoseconds(1_000_000_000 - now.nanosecond() as i64);

src/proto/h1/date.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@tomaka
Copy link

tomaka commented Jun 7, 2020

As someone who is experimenting with the wasm32-wasi target, time v0.1 is problematic because it does the time gathering itself. I have to use a local override of this crate at the moment to make things work.

time v0.2, on the other hand, delegates everything to std::time::SystemTime, which does work on wasi.

So if that's a valid argument, landing this PR would make hyper compile for Wasi.

@jhpratt
Copy link

jhpratt commented Jun 16, 2020

While I don't believe it should block this PR landing, a 0.3 is in progress, which will have very different formatting. It's not done yet, so I can't speak as to how well the compiler will optimize it away, but it should be very low cost, assuming the format is known at compile-time (which it sounds like it is).

@seanmonstar
Copy link
Member

I've filed #2253 to remove time 0.1. I was concerned about the "weight" of the new version of time, specifically it has an extra dependency, a procedural macro, and a bunch of time manipulation code that hyper didn't need. I've opted to use httpdate, which still uses SystemTime internally, but has much less code to compile.

@jhpratt
Copy link

jhpratt commented Jul 29, 2020

Just to address the concern about additional weight, time 0.3 will have far more aggressive feature flags. The macros will be fully rewritten to be lightweight (eliminating syn and quote) and being a flag. I also recently ran cargo diet to eliminate publishing unnecessary files, though that's not yet published (or even pushed up I believe).

The extra dependency is standback, which compiles down to literally nothing on the most recent stable — its sole purpose is to backport APIs, so all the compiler would have to do it tokenize and determine what compiler is being used via a build script.

I've actually considered putting the arithmetic behind an on-by-default flag, such that no-default-features would essentially leave you with just the struct definitions and inherent methods, nothing else. Formatting will be placed behind a flag in 0.3 as well.

Of course, httpdate is still probably more efficient, given that it's a fixed format. It's pretty difficult to surpass that, I'd imagine. Just throwing this out there to indicate that most of the issues with regard to weight will be improved! Thanks for your work on hyper and everything else.

@seanmonstar
Copy link
Member

That's excellent to hear!

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.

4 participants