Enable Fast Double Parser in Jackson

[mgodwan]

Description

Enable Jackson Features USE_FAST_DOUBLE_PARSER and USE_FAST_DOUBLE_WRITER

Related Issues

Resolves #7822

Check List

• New functionality includes testing.
  • All tests pass
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Verified all the content types with _doc API:

1. Smile
2. YAML
3. CBOR
4. JSON

JSON Document:

{ "account_number": 1,"address": "Some Avenue", "lat": 71.38  }

YAML Document:

---
account_number: 1
address: Some Avenue
lat: 71.38

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/16833/
• CommitID: 04c9caf
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/16834/
• CommitID: 802e679
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[dblock]

Let's add to the CHANGELOG something like "Improved performance of parsing ..."?

[reta]

@dblock @mgodwan I was looking into this feature and there are few "dragons" here:

• Measure performance improvement by "fast double parsing" FasterXML/jackson-core#970 (takeaway, with Java 17 there is negligible speed up of 1-2%)
• Allow use of faster floating-point number parsing (Schubfach) with StreamReadFeature.USE_FAST_DOUBLE_PARSER FasterXML/jackson-core#577 (takeaway, the results of the conversion may differ from the JDK)

I think this is a good feature to offer but may be we should be careful with using it as default?

[mgodwan]

@reta I ran a few benchmarks with nyc taxis where I saw the following differences in throughput without and with this change:

Code Path	Indexing Throughput Avg	P50 Indexing Latency
Baseline	78162.21,docs/s	876.34 ms
Candidate (Fast Double Parsing Enabled)	81743.31,docs/s (1.04x)	837.26 ms (0.956x)

These runs use JDK17 and show good improvements.

[mgodwan]

On the related note, we use Jackson for CBOR/Smile/Yaml (CborXContent/SmileXContent/YamlXContent), should we also switch those to USE_FAST_DOUBLE_WRITER? I think it would make sense.

I have updated the parser configuration for CborXContent, SmileXContent, and YamlXContent as well.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/17986/
• CommitID: b6e511e
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[reta]

@mgodwan conflicts :(

[mgodwan]

Resolved the conflicts.

[backslasht]

@mgodwan - Can you please add testing notes for the non JSON formats?

[reta]

@dblock are we good with making it default for main and 2.x but have an opt-out option for 2.x users (system property)?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      3 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

• URL: https://build.ci.opensearch.org/job/gradle-check/17987/
• CommitID: bf80f3d
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.search.SearchWeightedRoutingIT.testMultiGetWithNetworkDisruption_FailOpenEnabled

• URL: https://build.ci.opensearch.org/job/gradle-check/17988/
• CommitID: d34d085
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[dblock]

@dblock are we good with making it default for main and 2.x but have an opt-out option for 2.x users (system property)?

For 3.0 (main) looks good.

For 2.x, does it actually change behavior, even on edge cases? I read the above but can't tell what the specific examples are when users would get different results. If those exist, I would say it's a no-go because it would break semver. It should be optional.

[reta]

For 2.x, does it actually change behavior, even on edge cases? I read the above but can't tell what the specific examples are when users would get different results. If those exist, I would say it's a no-go because it would break semver. It should be optional.

Correct, we could address that at backport with system property

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.test.rest.ClientYamlTestSuiteIT.test {p0=search.aggregation/20_terms/string profiler via global ordinals}
      1 org.opensearch.gateway.RecoveryFromGatewayIT.testReuseInFileBasedPeerRecovery

• URL: https://build.ci.opensearch.org/job/gradle-check/18220/
• CommitID: 7beb7e2
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[mgodwan]

Thanks @dblock @reta

Could you help with merging this to main?
I can take up the required changes for backport once workflow is triggered.

[reta]

Could you help with merging this to main?

@dblock sign off?

[dblock]

I merged it, however I am still unclear what the edge cases are. I would at least add a test that demonstrates the handling of the edge case (a test that would fail if we remove this code).

[reta]

@dblock there is a difficult one (at least it seems to me):

• we could look for conversion edge cases (still have to find some), where JDK conversion != FastDoubleConversion, that could actually change from JDK to JDK (from the discussions above)
• we could look for conversion time edge cases (more like JMH-oriented test cases) where we assert that FastDoubleConversion should not take longer than X

Which ones you are thinking about? Or something very different?

[mgodwan]

we could look for conversion edge cases (still have to find some), where JDK conversion != FastDoubleConversion, that could actually change from JDK to JDK (from the discussions above)

For serialization, some cases which yield different output on jdk-17 and with jackson flag enabled are listed here.

e.g.

JDK 17 behaviour: 8.41E21 --> 8.409999999999999E21
JDK 20 behaviour: 8.41E21 --> 8.41E21
Jackson flag behaviour: 8.41E21 --> 8.41E21

I could not find any cases for JDK-20 since jdk-20 and jackson seem to be using the same algorithm for converting java double to string.

[mgodwan]

I merged it, however I am still unclear what the edge cases are. I would at least add a test that demonstrates the handling of the edge case (a test that would fail if we remove this code).

@dblock

• In this PR, we have only enabled jackson flag for converting from string to double[uses USE_FAST_DOUBLE_PARSER]. This yields better performance during document/query parsing cases.
• We have not enabled jackson implementation for double to string conversion [uses USE_FAST_DOUBLE_WRITER] since JDK-20 and jackson use the same algorithm, and we may not see any performance advantage (e.g. search query responses containing floating point numbers) with this flag in OS 3.0 which bundles with JDK 20
• The string to double procedure does not have any edge cases we are aware of (the algorithm follows IEEE standard) while there are edge cases in highlighted in double to string are covered in the above comment.

[dblock]

@mgodwan So you're saying that the two outcomes of this change are 1) improved performance, and 2) fixing a bug such as specifying 8.41E21 would produce 8.409999999999999E21? If so YOLO.