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

Use setRange when copying output chunks to the final buffer in CodedBufferWriter #887

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Oct 24, 2023

Similar to #885, this optimizes some more buffer copying to memcpy.

Results from the same benchmark in #885:

Before After Diff
AOT 114,713 us 109,838 us - 4,875 us, -4.2%
JIT 91,960 us 92,887 us + 927 us, +1.0%
dart2js -O4 259,125 us 257,000 us - 2,125 us, -0.8%
dart2wasm --omit-type-checks 196,909 us 182,333 us -14,576 us, -7.4%

AOT and JIT tested on x64.

cl/576132865

…edBufferWriter`

Similar to google#885, this optimizes some more buffer copying to `memcpy`.

Becuase the chunks can be very small, we use a simple loop as before on small
chunks and `setRange` on large chunks.

The chunk size to check for when to use a loop is somewhat arbitrarily chosen
as 20. In many benchmarks the chunks are either too small (less than 10 bytes)
or a more than a hundred bytes, so in the benchmarks I've tried it doesn't
matter whether the threshold is 20, 30, or 40.

The slowness in dart2js is probably caused by the `Uint8List` allocation, to be
passed to `setRange` as the source.

Results from the same benchmark in google#885. Before numbers are using the changes
in google#886.

|                              | Before     | After      | Diff                |
|------------------------------|------------|------------|---------------------|
| AOT                          | 122,741 us | 114,582 us | - 8,159 us, -6.6%   |
| JIT                          |  94,880 us |  92,317 us | - 2,483 us, -2.6%   |
| dart2js -O4                  | 258,250 us | 266,000 us | + 7,750 us, +3.0%   |
| dart2wasm --omit-type-checks | 195,300 us | 169,166 us | -26,134 us, -13.3%  |

AOT and JIT tested on x64.
@osa1 osa1 requested a review from mkustermann October 24, 2023 12:30
@osa1
Copy link
Member Author

osa1 commented Oct 24, 2023

@rakudrama any suggestions on how to improve the dart2js performance here? Martin's suggestion below fixed the issue.

@osa1 osa1 requested a review from mkustermann October 24, 2023 13:25
@osa1 osa1 merged commit 3528fad into google:master Oct 24, 2023
17 of 18 checks passed
@osa1 osa1 deleted the osa1/coded_buffer_writer_chunk_copying branch October 24, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants