Skip to content

Conversation

@Turnerj
Copy link
Collaborator

@Turnerj Turnerj commented Dec 25, 2019

Previously we would allocate (up to) 4 lists in the IEnumerable<object> constructor on Values<>. This is fine when we have all types supported by Values<> present in the JSON we are reading but is overkill when we have less than that.

This change is effectively just-in-time list construction where we only instantiate that type when we actually have an item of that type.

According to my benchmarks, there is a 1.5% decrease in allocations - likely as a best case. As a worst case (a Values4 has all 4 types present in JSON), the allocations should be the same.

@Turnerj Turnerj added the enhancement Issues describing an enhancement or pull requests adding an enhancement. label Dec 25, 2019
@Turnerj
Copy link
Collaborator Author

Turnerj commented Dec 25, 2019

There is an interesting quirk with this change - it now shows the problem I mentioned in #116 more obviously. With that in mind, this now depends on #116 - once that is merged, then I can rebase this on that and we can try again!

@RehanSaeed
Copy link
Owner

Happy to take this. Just got some failing tests.

@Turnerj Turnerj force-pushed the optimising-values-list-use branch from 2f95a30 to 706c482 Compare December 31, 2019 01:37
@Turnerj
Copy link
Collaborator Author

Turnerj commented Dec 31, 2019

I've rebased this on top of master now that the refactoring of OneOrMany is now merged - now the tests are passing.

@Turnerj Turnerj merged commit e41be22 into RehanSaeed:master Dec 31, 2019
@Turnerj Turnerj deleted the optimising-values-list-use branch December 31, 2019 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Issues describing an enhancement or pull requests adding an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants