Skip to content

Optimize ser/de to avoid using output stream#9278

Merged
Jackie-Jiang merged 2 commits intoapache:masterfrom
Jackie-Jiang:object_ser_de
Aug 25, 2022
Merged

Optimize ser/de to avoid using output stream#9278
Jackie-Jiang merged 2 commits intoapache:masterfrom
Jackie-Jiang:object_ser_de

Conversation

@Jackie-Jiang
Copy link
Contributor

Avoid using ByteArrayOutputStream which will potentially copy bytes multiple times (during expand and final toByteArray()). Pre-calculate the buffer size required, and then create the buffer only once.

Before optimization

BenchmarkObjectSerDe.bytesSet avgt 5 557.718 ± 31.501 ms/op
BenchmarkObjectSerDe.stringList avgt 5 1089.643 ± 30.302 ms/op
BenchmarkObjectSerDe.stringSet avgt 5 1061.030 ± 98.827 ms/op
BenchmarkObjectSerDe.stringToStringMap avgt 5 1947.736 ± 29.011 ms/op

After optimization

BenchmarkObjectSerDe.bytesSet avgt 5 360.085 ± 10.518 ms/op
BenchmarkObjectSerDe.stringList avgt 5 911.377 ± 35.301 ms/op
BenchmarkObjectSerDe.stringSet avgt 5 920.723 ± 29.300 ms/op
BenchmarkObjectSerDe.stringToStringMap avgt 5 1699.881 ± 165.362 ms/op

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2022

Codecov Report

Merging #9278 (7922b2a) into master (4f95945) will decrease coverage by 54.60%.
The diff coverage is 1.75%.

❗ Current head 7922b2a differs from pull request most recent head cab333d. Consider uploading reports for the commit cab333d to get more accurate results

@@              Coverage Diff              @@
##             master    #9278       +/-   ##
=============================================
- Coverage     69.93%   15.33%   -54.61%     
+ Complexity     5009      168     -4841     
=============================================
  Files          1861     1811       -50     
  Lines         99222    97124     -2098     
  Branches      15092    14861      -231     
=============================================
- Hits          69394    14895    -54499     
- Misses        24916    81096    +56180     
+ Partials       4912     1133     -3779     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 ?
unittests2 15.33% <1.75%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pinot/broker/api/resources/PinotClientRequest.java 0.00% <0.00%> (-43.29%) ⬇️
...r/requesthandler/BrokerRequestHandlerDelegate.java 32.25% <0.00%> (-20.25%) ⬇️
...requesthandler/MultiStageBrokerRequestHandler.java 0.00% <0.00%> (-74.08%) ⬇️
...mon/segment/generation/SegmentGenerationUtils.java 0.00% <0.00%> (-46.06%) ⬇️
...pache/pinot/common/utils/request/RequestUtils.java 0.00% <0.00%> (-82.90%) ⬇️
...org/apache/pinot/sql/parsers/CalciteSqlParser.java 0.00% <0.00%> (-87.11%) ⬇️
...rg/apache/pinot/sql/parsers/SqlNodeAndOptions.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/controller/api/resources/Constants.java 42.10% <ø> (ø)
...t/controller/api/resources/PinotQueryResource.java 0.00% <0.00%> (-55.08%) ⬇️
...oller/api/resources/PinotRunningQueryResource.java 0.00% <0.00%> (ø)
... and 1416 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

}
} catch (IOException e) {
throw new RuntimeException("Caught exception while serializing Map", e);
long bufferSize = (3 + 2 * (long) size) * Integer.BYTES;
Copy link
Contributor

@siddharthteotia siddharthteotia Aug 25, 2022

Choose a reason for hiding this comment

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

(nit) suggest giving a brief comment (for future readers) on what 3 and 2 means. I think

3 -> size, keyTypeValue, valueTypeValue
2-> keyBytes.length, valueBytes.length

@Jackie-Jiang Jackie-Jiang merged commit d778df2 into apache:master Aug 25, 2022
@Jackie-Jiang Jackie-Jiang deleted the object_ser_de branch August 25, 2022 17:32
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.

3 participants