Skip to content

Conversation

@RanVaknin
Copy link
Contributor

@RanVaknin RanVaknin commented Oct 23, 2025

🛑 Blocked until Jackson 2.19.3 is released

This PR depends on the COMBINE_UNICODE_SURROGATES_IN_UTF8 feature introduced in Jackson 2.18.0. The SDK currently uses an older Jackson version that lacks this feature. Because of the sensitive nature of Jackson upgrade, non trivial testing must be done to ensure backwards compatibility is maintained. Thus, we are consolidating upgrading efforts until Jackson 2.19.3 is released to minimize testing overhead from multiple version bumps.


This PR addresses a performance degradation issue between v1 and v2. After considering multiple implementation approaches, the proposed optimization approach yielded significant reduction in latency across the board.

toJson() Benchmarks

Metrics p0.90 p0.95 p0.99 Change (p90)
v1 611µs 634µs 739µs baseline
v2 Current 2,953µs 2,527µs 2,404µs +383%
**v2 SDKJsonGenerator - writes to byteArrayOutputStream 550µs 564µs 613µs - 10%
v2 JsonGenerator - writes to stringWriter 680µs 719µs 1541µs +11%

getJson() Benchmarks

50 bytes payload

Metrics p0.50 p0.90 p0.99 Change (p90)
v1 0.125µs 0.167µs 0.209µs baseline
v2 current 0.291µs 0.292µs 0.417µs +75%
v2 Optimized 0.167µs 0.250µs 0.458µs +50%

100 bytes payload

Metrics p0.50 p0.90 p0.99 Change (p90)
v1 0.208µs 0.209µs 0.292µs baseline
v2 current 0.708µs 0.833µs 1.082µs +299%
v2 Optimized 0.250µs 0.292µs 0.417µs +40%

500 bytes payload

Metrics p0.50 p0.90 p0.99 Change (p90)
v1 0.542µs 0.584µs 0.750µs baseline
v2 current 2.292µs 2.416µs 3.084µs +314%
v2 Optimized 0.625µs 0.667µs 0.833µs +14%

1KB payload

Metrics p0.50 p0.90 p0.99 Change (p90)
v1 1.250µs 1.374µs 1.708µs baseline
v2 current 5.040µs 5.456µs 6.792µs +297%
v2 Optimized 1.332µs 1.416µs 1.792µs +3%

High level Solution

Current Flow:

AttributeValue → JsonNode (via JsonItemAttributeConverter) → String (via JsonStringFormatHelper) → Stream joining

Proposed Flow:

AttributeValue → SDKJsonGenerator → String

General Optimization Approach

Instead of converting each AttributeValue to a JsonNode and then converting JsonNode to string via costly string builder and stream collection , we serialize AttributeValues directly using SDKJsonGenerator methods which delegates all escaping and buffer management to Jackson core.

SdkJsonGenerator (ByteArrayOutputStream) vs JsonGenerator (StringWriter)

Tested both serialization approaches with identical enum dispatch logic:
SdkJsonGenerator wraps JsonFactory.createGenerator(ByteArrayOutputStream) → creates UTF8JsonGenerator - 19% faster (550µs vs 680µs at p90)
JsonGenerator with JsonFactory.createGenerator(StringWriter) → creates WriterBasedJsonGenerator

⚠️ Potential behavior change

UTF8JsonGenerator escapes non ASCII as \uXXXX by default. Jackson 2.18+ provides COMBINE_UNICODE_SURROGATES_IN_UTF8 feature to output literal UTF-8 (verified via test), but the SDK bundles older shaded Jackson version that lacks this feature.
see FasterXML/jackson-core#223

@sonarqubecloud
Copy link

@RanVaknin RanVaknin force-pushed the rvaknin/optimize-to-json-performance branch from c0c396a to 6b2036a Compare October 27, 2025 06:45
@RanVaknin RanVaknin changed the title adding strategy serialization optimization Serialization optimization for DDB enhanced Client Oct 28, 2025
@dagnir
Copy link
Contributor

dagnir commented Oct 29, 2025

BTW can you take this out of draft?

@dagnir
Copy link
Contributor

dagnir commented Oct 29, 2025

This PR depends on the COMBINE_UNICODE_SURROGATES_IN_UTF8 feature introduced in Jackson 2.18.0.

We should have a test for this, doesn't look like we have one unless I missed it!

@RanVaknin RanVaknin marked this pull request as ready for review October 29, 2025 18:14
@RanVaknin RanVaknin requested a review from a team as a code owner October 29, 2025 18:14
@RanVaknin RanVaknin added the api-surface-area-approved-by-team Indicate API surface area introduced by this PR has been approved by team label Oct 30, 2025
@RanVaknin
Copy link
Contributor Author

@dagnir do we need a changelog? its an internal implementation - so should be invisible?

@dagnir
Copy link
Contributor

dagnir commented Oct 30, 2025

@dagnir do we need a changelog? its an internal implementation - so should be invisible?

Good question; yes we should have an entry for this. It's a significant performance improvement that customers may be interested in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-surface-area-approved-by-team Indicate API surface area introduced by this PR has been approved by team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants