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

Add optimized supplier of current HTTP timestamp #2196

Merged
merged 5 commits into from
Oct 19, 2019

Conversation

anuraaga
Copy link
Collaborator

As TechEmpower suggests, this caches HTTP timestamps for any given second. I can wire this into the Date header after that's in if this makes sense :)

Benchmark                                  Mode  Cnt         Score         Error  Units
HttpTimestampSupplierBenchmark.cached     thrpt    5  37665728.115 ▒  324874.523  ops/s
HttpTimestampSupplierBenchmark.notCached  thrpt    5   2099035.297 ▒ 1176888.041  ops/s

@anuraaga anuraaga force-pushed the fast-http-timestamp branch from 481b46a to 8a245bd Compare October 18, 2019 06:29
public Instant get() {
return Instant.now();
public String get() {
return HttpTimestampSupplier.currentTime();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could remove this class and just use HttpTimestampSupplier::currentTime?

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #2196 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2196      +/-   ##
============================================
- Coverage     73.61%   73.61%   -0.01%     
- Complexity     9590     9591       +1     
============================================
  Files           837      838       +1     
  Lines         36936    36944       +8     
  Branches       4553     4554       +1     
============================================
+ Hits          27190    27195       +5     
- Misses         7420     7426       +6     
+ Partials       2326     2323       -3
Impacted Files Coverage Δ Complexity Δ
...a/com/linecorp/armeria/server/cors/CorsPolicy.java 65.71% <ø> (ø) 19 <0> (ø) ⬇️
...a/com/linecorp/armeria/server/cors/CorsConfig.java 72.41% <ø> (-0.17%) 19 <0> (ø)
...necorp/armeria/internal/HttpTimestampSupplier.java 100% <100%> (ø) 5 <5> (?)
...inecorp/armeria/server/HttpResponseSubscriber.java 84.18% <100%> (-1.41%) 73 <0> (-3)
...om/linecorp/armeria/client/HttpSessionHandler.java 61.01% <0%> (-11.02%) 29% <0%> (-4%)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 72.81% <0%> (+0.48%) 49% <0%> (+1%) ⬆️
.../main/java/com/linecorp/armeria/server/Server.java 83.92% <0%> (+1.17%) 29% <0%> (ø) ⬇️
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 86.31% <0%> (+2.1%) 15% <0%> (+1%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3969bf...c114bdc. Read the comment docs.

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍

@trustin trustin added this to the 0.95.0 milestone Oct 19, 2019
newHeaders.add(HttpHeaderNames.DATE,
DateTimeFormatter.RFC_1123_DATE_TIME.format(
ZonedDateTime.now(ZoneOffset.UTC)));
newHeaders.add(HttpHeaderNames.DATE, HttpTimestampSupplier.currentTime());
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! /cc @jyblue

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @anuraaga
This PR gave me a good education 👍

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

LGTM Thanks, @anuraaga

@trustin trustin merged commit cca8e3c into line:master Oct 19, 2019
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

muy bueno

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
As TechEmpower suggests, this caches HTTP timestamps for any given second. I can wire this into the `Date` header after that's in if this makes sense :)

```
Benchmark                                  Mode  Cnt         Score         Error  Units
HttpTimestampSupplierBenchmark.cached     thrpt    5  37665728.115 ▒  324874.523  ops/s
HttpTimestampSupplierBenchmark.notCached  thrpt    5   2099035.297 ▒ 1176888.041  ops/s
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants