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.

Sign up for GitHub

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

Jump to bottom

Speed up converting of temporal accessor to zoned date time #37915

Merged
merged 13 commits into from
Jan 31, 2019
Merged

Speed up converting of temporal accessor to zoned date time #37915

merged 13 commits into from
Jan 31, 2019

Conversation

spinscale
Copy link
Contributor

The existing implementation was slow due to exceptions being thrown if
an accessor did not have a time zone. This implementation queries for
having a timezone, local time and local date and also checks for an
instant preventing to throw an exception and thus speeding up the conversion.

This removes the existing method and create a new one named
DateFormatters.from(TemporalAccessor accessor) to resemble the naming of
the java time ones.

Before this change an epoch millis parser using the toZonedDateTime
method took

DateFormatterToZonedTimeBenchmark.benchmarkToZonedDateTime avgt 30 3970,202 ± 71,260 ns/op

With this change

DateFormatterToZonedTimeBenchmark.benchmarkToZonedDateTime avgt 30 67,863 ± 1,162 ns/op

Closes #37826

@spinscale
Speed up converting of temporal accessor to zoned date time
6a997f3 
The existing implementation was slow due to exceptions being thrown if
an accessor did not have a time zone. This implementation queries for
having a timezone, local time and local date and also checks for an
instant preventing to throw an exception and thus speeding up the conversion.

This removes the existing method and create a new one named
DateFormatters.from(TemporalAccessor accessor) to resemble the naming of
the java time ones.

Before this change an epoch millis parser using the toZonedDateTime
method took

DateFormatterToZonedTimeBenchmark.benchmarkToZonedDateTime  avgt   30  3970,202 ± 71,260  ns/op

With this change

DateFormatterToZonedTimeBenchmark.benchmarkToZonedDateTime  avgt   30  67,863 ± 1,162  ns/op

Closes elastic#37826
@spinscale spinscale added >regression :Core/Infra/Core Core issues without another label v7.0.0 labels Jan 28, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@spinscale
Copy link
Contributor Author

spinscale commented Jan 28, 2019 •
edited
Loading

@elasticmachine run elasticsearch-ci/default-distro

danielmitterdorfer
danielmitterdorfer reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

I left a few comments.

server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java Outdated
@@ -662,7 +662,8 @@ private void assertSameDate(String input, String format, DateFormatter jodaForma
DateTime jodaDateTime = jodaFormatter.parseJoda(input);

TemporalAccessor javaTimeAccessor = javaFormatter.parse(input);
ZonedDateTime zonedDateTime = DateFormatters.toZonedDateTime(javaTimeAccessor);
// ZonedDateTime zonedDateTime = DateFormatters.toZonedDateTime(javaTimeAccessor);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, removed

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/DateFormat.java Outdated
// if there is no year, we fall back to the current one and
// fill the rest of the date up with the parsed date
if (accessor.isSupported(ChronoField.YEAR) == false) {
ZonedDateTime newTime = Instant.EPOCH.atZone(ZoneOffset.UTC).withYear(year);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use the "current" year in the specified time zone instead of UTC? Wouldn't we otherwise either use the next or the previous year around the turn of the year?

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 resembled the joda time behaviour here. I also created a test that compares joda time with java time to make sure the output is the same

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java Outdated
*
* @return The converted zoned date time
*/
@SuppressForbidden(reason = "LocalDate.of is fine here")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we should extract the access to LocalDate#of to a dedicated method so we can reduce the scope of the SuppressForbidden annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally. done.

danielmitterdorfer
danielmitterdorfer approved these changes Jan 29, 2019
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. LGTM.

pgomulka
pgomulka approved these changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM - I really like this change. I only left one comment which I supposed you already spotted with Daniel

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
@@ -1544,105 +1552,86 @@ static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
dateTimeFormatters.toArray(new DateTimeFormatter[0]));
}

private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC);
private static final LocalDate LOCALDATE_EPOCH = LocalDate.of(1970, 1, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an annotation below @SuppressForbidden(reason = "LocalDate.of is fine here")
Why is it forbidden ? Should be also then applied here?
i suppose that maybe this was already discussed comment

@spinscale spinscale merged commit b94acb6 into elastic:master Jan 31, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 31, 2019
@jasontedor
Merge branch 'master' into retention-leases-version
4aa6b77 
* master: (100 commits)
  Push primary term to replication tracker (elastic#38044)
  Introduce ability to minimize round-trips in CCS (elastic#37828)
  Don't Assert Ack on when Publish Timeout is 0 in Test (elastic#38077)
  Reduce object creation in Rounding class (elastic#38061)
  Treat put-mapping calls with `_doc` as a top-level key as typed calls. (elastic#38032)
  Fix test bug when testing the merging of mappings and templates. (elastic#38021)
  spelling: java script -- not JavaScript (elastic#37057)
  Enable SSL in reindex with security QA tests (elastic#37600)
  Disable BWC tests during backport (elastic#38074)
  SQL: Added SSL configuration options tests (elastic#37875)
  Minor fixes in the release notes script. (elastic#37967)
  Fix typo in docs. (elastic#38018)
  Update Lucene repo for 7.0.0-alpha2 (elastic#37985)
  Fix size of rolling-upgrade bootstrap config (elastic#38031)
  fix DateIndexNameProcessorTests offset pattern (elastic#38069)
  Speed up converting of temporal accessor to zoned date time (elastic#37915)
  Work around JDK8 timezone bug in tests (elastic#37968)
  Correct arg names when update mapping/settings from leader (elastic#38063)
  Introduce ssl settings to reindex from remote (elastic#37527)
  Mute testRetentionLeasesSyncOnExpiration
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgomulka pgomulka pgomulka approved these changes

@danielmitterdorfer danielmitterdorfer danielmitterdorfer approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >regression v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spinscale @elasticmachine @danielmitterdorfer @pgomulka @colings86