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

Drop in indexing performance and increase in agg latency after mappings/aggs switch to java time in #36363 #37826

Closed
dliappis opened this issue Jan 24, 2019 · 2 comments

Comments

Projects
None yet
7 participants
@dliappis
Copy link
Contributor

commented Jan 24, 2019

#36363 (merged to master in daa2ec8) has caused a performance drop manifested in both a drop in indexing throughput (see: https://elasticsearch-benchmarks.elastic.co/#tracks/nyc-taxis/nightly/30d for http_logs / nyc_taxis / nested / noaa) as well as latency increase in aggs (see: nyc_taxis, nested, http_logs) and gc (nested) on 2018-01-23.

  • Before: nyc_taxis median indexing throughput: 81421 docs/s
  • After: nyc_taxis median indexing throughput: 65981 docs/s

Reproduction steps:

Java time commit daa2ec8:

esrally --pipeline="from-sources-complete" --track="nyc_taxis" --car="4gheap" --runtime-jdk=8 --challenge="append-no-conflicts" --revision="daa2ec8" --include-tasks="delete-index,create-index,check-cluster-health,index,refresh-after-index"

Previous commit 7b3dd30:

esrally --pipeline="from-sources-complete" --track="nyc_taxis" --car="4gheap" --runtime-jdk=8 --challenge="append-no-conflicts" --revision="7b3dd30" --include-tasks="delete-index,create-index,check-cluster-health,index,refresh-after-index"

Note: If you are using Oracle JDK you can also get a profile by specifying --telemetry=jfr.

@elasticmachine

This comment has been minimized.

Copy link

commented Jan 24, 2019

@dliappis dliappis added the v7.0.0 label Jan 24, 2019

@jpountz jpountz added the blocker label Jan 24, 2019

spinscale added a commit to spinscale/elasticsearch that referenced this issue Jan 28, 2019

Speed up converting of temporal accessor to zoned date time
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 added a commit that referenced this issue Jan 31, 2019

Speed up converting of temporal accessor to zoned date time (#37915)
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 approximately 50x longer.

Relates #37826

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 1, 2019

Speed up converting of temporal accessor to zoned date time
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 approximately 50x longer.

Relates elastic#37826

This commit also backports elastic#38171

spinscale added a commit that referenced this issue Feb 1, 2019

Speed up converting of temporal accessor to zoned date time (#38172)
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 approximately 50x longer.

Relates #37826

This commit also backports #38171
@spinscale

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

this is still open for the aggs case, but I have a fix ready... running benchmarks at the moment

@spinscale spinscale reopened this Feb 1, 2019

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 1, 2019

Restore date aggregation performance in UTC case
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

@danielmitterdorfer danielmitterdorfer added v7.0.0 and removed v8.0.0 labels Feb 8, 2019

spinscale added a commit that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes #37826

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (elastic#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (elastic#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (elastic#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826

spinscale added a commit to spinscale/elasticsearch that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (elastic#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826

spinscale added a commit that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (#38221) (#38700)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes #37826

spinscale added a commit that referenced this issue Feb 11, 2019

Restore date aggregation performance in UTC case (#38221) (#38701)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes #37826

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 12, 2019

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this issue Feb 12, 2019

Restore date aggregation performance in UTC case (elastic#38221)
The benchmarks showed a sharp decrease in aggregation performance for
the UTC case.

This commit uses the same calculation as joda time, which requires no
conversion into any java time object, also, the check for an fixedoffset
has been put into the ctor to reduce the need for runtime calculations.
The same goes for the amount of the used unit in milliseconds.

Closes elastic#37826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.