Skip to content

Conversation

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jun 18, 2021

This PR backports a selection of bugfixes, infrastructural changes and perf improvements that were made as part of the polymorphism feature branch in #53882. I have layered the changes into separate commits that should ideally be reviewed independently:

  1. a858d3b fixes a series of bugs that were discovered while prototyping.
  2. 99529f7 adds debug assertions around Push() and Pop() operations that assist in early detection of stack-corrupting bugs.
  3. 4ba2497 reworks and simplifies the WriteStack and ReadStack implementations to
    • Use arrays instead of lists for storing previous stackframes: previous frames can now be passed by reference where needed.
    • Ensures that the _count field always accurately reflects the current stack depth.
    • Removes the StackFrame.Reset() methods and instead clears Current by assigning it to a default instance. This eliminates a class of bugs caused by dirty frames being reused in new contexts.
  4. cb59430 takes advantage of recently added JIT optimizations (e.g. optimizing typeof(T).IsValueType expressions Optimize typeof(T).IsValueType #1157) in the serialization hot path methods.

The changes should make it easier to incorporate the polymorphism infrastructural changes in the future. Overall the changes result in a slight increase in serialization performance, on average between 4-8% faster in our benchmark suite.

* Use array instead of list for storing frames. Previous frames can now be passed by reference
* Ensure that the _count field always reflects the current depth.
* Remove StackFrame.Reset() methods which are occassionally a source of dirty frames being reused.
@ghost
Copy link

ghost commented Jun 18, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR backports a selection of bugfixes, infrastructural changes and perf improvements that were made as part of the polymorphism feature branch in #53882. I have layered the changes into separate that should ideally be reviewed independently:

  1. a858d3b fixes a series of bugs that were discovered while prototyping.
  2. 99529f7 adds debug assertions around Push() and Pop() operations that assist in early detection of stack-corrupting bugs.
  3. 4ba2497 reworks and simplifies the WriteStack and ReadStack implementations to
    • Use arrays instead of lists for storing previous stackframes: previous frames can now be passed by reference where needed.
    • Ensures that the _count field always accurately reflects the current stack depth.
    • Removes the StackFrame.Reset() methods and instead clears Current by assigning it to a default instance. This eliminates a class of bugs caused by dirty frames being reused in new contexts.
  4. cb59430 takes advantage of recently added JIT optimizations (e.g. optimizing typeof(T).IsValueType expressions Optimize typeof(T).IsValueType #1157) in the serialization hot path methods.

The changes should make it easier to incorporate the polymorphism infrastructural changes in the future. Overall the changes result in a slight increase in serialization performance, on average between 4-8% faster in our benchmark suite.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 6.0.0

Copy link
Contributor

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring. LGTM with a couple questions.

@ghost
Copy link

ghost commented Jun 18, 2021

Hello @eiriktsarpalis!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 6737dce into dotnet:main Jun 18, 2021
@eiriktsarpalis eiriktsarpalis deleted the backport-polymorphism-fixes branch June 18, 2021 20:45
@ghost ghost locked as resolved and limited conversation to collaborators Jul 18, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants