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

Perf Fix CreateEtags #1128

Merged

Conversation

ElizabethOkerio
Copy link
Contributor

@ElizabethOkerio ElizabethOkerio commented Dec 6, 2023

This PR fixes some of the issues raised in this PR: #1111:
I've made the following updates to the CreateEtag method:

  • Set the initial value of the properties dictionary to null and initialize properties only within the foreach loop. This approach guarantees that the dictionary is instantiated only when concurrency properties are present.
  • I excluded the OrderBy operation from the IEnumerable of concurrency properties to eliminate the cost associated with OrderedEnumerable.MoveNext(). Consequently, the existing approach retrieves the concurrent properties, orders them, and subsequently inserts them into a dictionary. If the dictionary preserves the order, the crucial factor lies in the manner in which properties are added to the dictionary rather than whether they are sorted before being added to the dictionary. @xuzhg what are your thoughts on this?
  • Added a null check for the properties, ensuring that the CreateEtag method is invoked only when the properties are non-null.
    These changes have the following CPU Usage improvements:
    Before changes:
beforeetag

After changes:
createetag

@ElizabethOkerio ElizabethOkerio marked this pull request as ready for review December 6, 2023 08:24
@ElizabethOkerio ElizabethOkerio force-pushed the perf-fix/create-etag-optimizations branch from f9641d8 to a710d78 Compare December 6, 2023 09:22
xuzhg
xuzhg previously approved these changes Dec 7, 2023
foreach (IEdmStructuralProperty etagProperty in concurrencyProperties)
{
properties ??= new Dictionary<string, object>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel a certain type of way about checking whether the properties dictionary is null in each iteration? More CPU cycles and feels like we're sacrificing one thing for the other. If we must go this route, then the following would also be candidate:

IEnumerator<IEdmStructuralProperty> concurrencyPropertiesEnumerator = concurrencyProperties.GetEnumerator());

if (concurrencyPropertiesEnumerator.MoveNext())
{
    properties = new Dictionary<string, object>();

    do
    {
        IEdmStructuralProperty etagProperty = concurrencyPropertiesEnumerator.Current;
        properties.Add(etagProperty.Name, resourceContext.GetPropertyValue(etagProperty.Name));
    }
    while (concurrencyPropertiesEnumerator.MoveNext());
}

At least this way we're not checking if properties dictionary is null in each iteration.
Whether the code is clean and easy-to-follow is a different matter altogether...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most cases we do not have concurrency properties..

@ElizabethOkerio ElizabethOkerio force-pushed the perf-fix/create-etag-optimizations branch from 10c21cd to a73bd98 Compare December 8, 2023 13:59
foreach (IEdmStructuralProperty etagProperty in concurrencyProperties)
{
properties ??= new SortedDictionary<string, object>();
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I think gathogo's suggestion of using MoveNext() instead of doing a null-check on each iteration wasn't that bad. But since this code path isn't that common, not sure it makes material difference.

@ElizabethOkerio ElizabethOkerio merged commit c4de3fa into OData:main Jan 10, 2024
2 checks passed
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.

5 participants