Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add DuplicatePropertyNameChecker to ObjectPool #2328
Add DuplicatePropertyNameChecker to ObjectPool #2328
Changes from 18 commits
b8f75ec
8dff302
9ecf332
505af7e
4a03fbf
884059f
23359a8
24ed770
6facb2f
e0ea495
c351053
eea468b
734388a
16791b4
7d481f7
1f3e72c
abb2f5c
d103f5b
39dffb8
a78ceaf
6c8be85
409faea
2a27da5
4afdaa5
2b95462
0090551
af6293b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be trivializing this, but why isn't this the implementation:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's a good idea to throw an exception when we're out of slots. I think in that case we should either "resize" the array (which is maybe more costly than we want to pay) or create a new object that's not stored in the pool. If there's more demand than what can fit in the pool, then the excess is not pooled, but doesn't throw either. My 2 cents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've consolidated my different feedback threads into this one because I feel like something is getting lost.
ObjectPool
should have in its contract that it throws when it runs out of space. We should catch this exception somewhere else (probably in theODataSerializer
). If we don't throw an exception and we increase the nesting level, we will never notice that we are losing out on this optimization. I think we should either use a dynamically sized object pool, or we should throw an exception that we surface inDebug
configurations and don't surface inRelease
configurations.EDIT: another note on this, the case I'm describing here is just the obvious example of nesting levels change. Any bug that returns an extra object will cause the count to be off, and we will start leaking instances and overload the garbage collector just as we do today.
Ultimately, I think we should be using the .NET object pool implementation and we don't use an object pool for versions that don't have it. This should incentivize users to move to newer versions of .NET in order to receive these performance benefits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corranrogue9
netstandard 1.1
. Having 2 types object pools in ODL now only adds extra complexity.Conclusion: This PR was intended on to reduce heap allocation. At the same time not regress cpu time. Further optimizations can be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. It throws an exception.
Yes we should. We need to notice when we have made an unrelated change that causes us to go beyond the current nesting level and therefore lose out on this optimization. Please address this concern that I have: if we don't throw an exception, how do we, as a team, know that we have inadvertently increased the nesting level?
What is the benefit of doing this? From what I see, it only serves to hide potential bugs.
The .NET implementation has an extra requirement of being thread-safe, meaning that
Interlocked.Exchange
is imperative to its functioning. Because of this, reference types are needed and keeping a reference to the next item is essential. Without that requirement, we can improve performance by using a different algorithm rather than by optimizing the .NET algorithm.Please create an issue for this so that the work can be planned for and assigned
I am not proposing that we have 2. I am proposing that we don't ship this optimization for older versions of .NET. This will incentivize customers to use newer versions of .NET. Alternatively, we could simply take the .NET implementation and place it directly into our repo, referencing where it came from. Then, we can ship down-level, but have complete consistency across versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to throw an exception, because that's implying that having responses with depths that exceed the object pool size is an error. The size of the object pool shouldn't impose a limit on the depth of the response (there's a separate setting for that).
If the goal of the exception is for sanity checks to ensure we are not exceeding the object pool size due a bug in our code rather than due to the depth of the response, maybe we could have debug messages or assertions when the object pool size is exceeded, when the number of returned objects is not equal the number of requested objects, etc.
Or did you mean that we conditionally throw exceptions based on
#if DEBUG
or something like that?I prefer the simpler implementation provided by @corranrogue9, but I still think it's valuable to delay allocating the array until the second element is requested, that way we avoid unnecessary allocations for non-nested responses. Unless we can demonstrate that the cost of having the
if
statement that's always checked is worse.Also, since the point of this PR is to reduce allocations (and GC), I think it's okay to only care about reference types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is where you should create the array if it's null:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what was here before, but there's really no need to have an instance of
ObjectPool
with anull
value foritems
. I don't see us ever set it back tonull
, so we could mark itreadonly
if we initialize it in the constructor. Also, doing this means that we will check the conditional every timeGet
is called, which seems suboptimal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about calling
if (items == null)
on everyGet()
call seeming suboptimal. My suggestion was based on eliminating an array allocation (and deallocation) when it's not necessary. I would assume it would be much easier for JIT or CPU to optimizeif (items == null)
away via branch prediction or some other techniques ifitems == null
evaluates to the same value most of the times. So in both cases there's trade-off to be made. But this lazy initialization pattern seems common in BCL source code, e.g.:Can this be evaluated using benchmarks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we quantify this performance benefit for our scenarios? I strongly consider this an anti-pattern, given that we can never actually refer to an individual
ObjectWrapper
without introducing impossible to find bugs. Just this line from theReturn
methoditems[i].Element = obj;
would not work if written asvar wrapper = items[i]; wrapper.Element = obj
even though they look to be doing identical things.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By avoiding runtime checks, we reduce CPU time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a noticeable CPU difference in profiler or benchmarks with and without
ObjectWrapper
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some sort of numbers is really necessary to justify introducing this complexity.