Skip to content

Several improvements to PostData #4312

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

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Jan 9, 2020

New types

  • ReadOnlyMemory, only available in netstandard2.1 allows you to
    post ReadOnlyMemory<byte>
  • StreamHandler allows you to inject a stream handler and write to the
    request streams the best way you see fit yourself

Performance

  • PostData.String now reuses bytes
  • PostData.MultiJson(string[]) uses way less allocations

Benchmarks current PR posting 10.000 objects (many megabytes overal in size) in various ways.

Method Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
ListOfObjects 449.20 ms 8.854 ms 13.785 ms 447.42 ms 11000.0000 - - 74.89 MB
StaticListOfObjects 437.08 ms 8.321 ms 7.784 ms 437.84 ms 11000.0000 - - 74.58 MB
PrecomputedPostDataOfObjects 435.43 ms 8.643 ms 17.061 ms 437.11 ms 11000.0000 - - 74.57 MB
StaticListOfStrings 48.20 ms 1.066 ms 3.042 ms 47.25 ms 4000.0000 - - 32.64 MB
PrecomputedPostDataOfStrings 47.48 ms 1.084 ms 3.076 ms 46.58 ms 4000.0000 - - 32.64 MB
PostDataString 48.30 ms 2.238 ms 6.598 ms 49.59 ms - - - 31.97 MB
PrecomputedPostDataString 29.05 ms 0.579 ms 1.546 ms 28.98 ms - - - 7.95 MB
PostDataByteArray 28.50 ms 0.566 ms 1.521 ms 28.30 ms - - - 7.95 MB
PostDataReadOnlyMemory 28.43 ms 0.562 ms 1.070 ms 28.29 ms - - - 7.95 MB
PostDataStreamHandler 28.89 ms 0.576 ms 1.240 ms 28.77 ms - - - 7.95 MB

Compare with current master:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ListOfObjects 577.61 ms 22.658 ms 33.212 ms 11000.0000 - - 74.9 MB
StaticListOfObjects 564.67 ms 16.437 ms 24.093 ms 11000.0000 - - 73.78 MB
PrecomputedPostDataOfObjects 486.68 ms 30.829 ms 46.143 ms 11000.0000 - - 73.86 MB
StaticListOfStrings 184.01 ms 5.788 ms 8.483 ms 8000.0000 3000.0000 - 201.65 MB
PrecomputedPostDataOfStrings 182.36 ms 4.817 ms 7.061 ms 8000.0000 3000.0000 - 201.88 MB
PostDataString 59.34 ms 5.446 ms 8.152 ms - - - 56.01 MB
PrecomputedPostDataString 60.56 ms 5.526 ms 8.271 ms - - - 56.12 MB
PostDataByteArray 33.31 ms 1.265 ms 1.894 ms - - - 7.95 MB

New types

* `ReadOnlyMemory`, only available in `netstandard2.1` allows you to
post `ReadOnlyMemory<byte>`
* `StreamHandler` allows you to inject a stream handler and write to the
request streams the best way you see fit yourself

Performance

* `PostData.String` now reuses bytes
* `PostData.MultiJson(string[]) uses way less allocations
Expose reusable stream handling on `PostData`

Add tests for new types so that they behave and write as expected
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I left some small comments.

Reading through the post data docs, I think we should consider revising the format of them in a future PR, to focus more on how PostData can be used with regard to the rest of the client. Happy to open a PR for this.

ms = !string.IsNullOrEmpty(_literalString) ? settings.MemoryStreamFactory.Create(_literalString.Utf8Bytes()) : null;
if (string.IsNullOrEmpty(_literalString)) return;

var stringBytes = WrittenBytes ?? _literalString.Utf8Bytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is WrittenBytes checked first in the event this is a retry with the same PostData instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, since the string is immutable we allow for PostData.String to be cached. We could potentially clear _literalString afterwards.

@Mpdreamz
Copy link
Member Author

Addressed PR feedback on follow up PR: #4313

Thanks for the review @russcam 👍

@Mpdreamz Mpdreamz merged commit af93854 into master Jan 10, 2020
@Mpdreamz Mpdreamz deleted the feature/master/postdata-improvements branch January 10, 2020 08:25
Mpdreamz added a commit that referenced this pull request Jan 10, 2020
* Several improvements to PostData

New types

* `ReadOnlyMemory`, only available in `netstandard2.1` allows you to
post `ReadOnlyMemory<byte>`
* `StreamHandler` allows you to inject a stream handler and write to the
request streams the best way you see fit yourself

Performance

* `PostData.String` now reuses bytes
* `PostData.MultiJson(string[]) uses way less allocations

* Move StreamHandler in to its own type.

Expose reusable stream handling on `PostData`

Add tests for new types so that they behave and write as expected

* update post-data docs

* Refactor SerializableData and reuse methods from PostData

(cherry picked from commit af93854)
Mpdreamz added a commit that referenced this pull request Jan 10, 2020
* Several improvements to PostData (#4312)

* Several improvements to PostData

New types

* `ReadOnlyMemory`, only available in `netstandard2.1` allows you to
post `ReadOnlyMemory<byte>`
* `StreamHandler` allows you to inject a stream handler and write to the
request streams the best way you see fit yourself

Performance

* `PostData.String` now reuses bytes
* `PostData.MultiJson(string[]) uses way less allocations

* Move StreamHandler in to its own type.

Expose reusable stream handling on `PostData`

Add tests for new types so that they behave and write as expected

* update post-data docs

* Refactor SerializableData and reuse methods from PostData

(cherry picked from commit af93854)

* Additional UTF8Json overloads, consolidate how we tell it to write/serialize. (#4313)

* Several improvements to PostData

New types

* `ReadOnlyMemory`, only available in `netstandard2.1` allows you to
post `ReadOnlyMemory<byte>`
* `StreamHandler` allows you to inject a stream handler and write to the
request streams the best way you see fit yourself

Performance

* `PostData.String` now reuses bytes
* `PostData.MultiJson(string[]) uses way less allocations

* Adds more write overloads to Utf8Json

Either memory stream or to serialize on the writer.

This consolidates and funnels places that need to write
bytes/streams/objects directly

(cherry picked from commit de62914)

* add missing hunk after git add -p session

(cherry picked from commit fc289b1)

* add comment explaining the check for possible fast path in sourceformatter

(cherry picked from commit ffc4e46)

* Move fastpath check into method and use it for Deserialize too

(cherry picked from commit f365b30)

* update formatters to use a better check for fast path

(cherry picked from commit b07e0a8)

* Move StreamHandler in to its own type.

Expose reusable stream handling on `PostData`

Add tests for new types so that they behave and write as expected

* Zap IInternalSerializerWithFormatter

* Make sure WriteRaw for memory streams assumes a memorycopy

* Make sure final newline is written again for bulk requests

* Address PR feedback

* reoorder variable assignments

(cherry picked from commit fadef66)
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.

2 participants