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

Optimize ODataPathExtensions.GetNavigationSource #1161

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

habbes
Copy link
Contributor

@habbes habbes commented Jan 23, 2024

Fix #1118

Replaces the use of ODataPathNavigationSourceHandler in ODataPath.GetNavigationSource extension method with a simpler reverse loop of the path.

With ODataPathNavigationSourceHandler

  • a new instance of the handler is allocated
  • all the segments of the path are visited
  • a List<string> is created and populated with the string representation of each path segment. This list is used to generate a string representation of the path via a Path property. But this property is not needed to determine the navigation source (it's also not used by existing code).

The new implementation scans the path segments from the end and stops as soon as it finds the navigation source because only the last navigation source in the path matters.

The ODataPathNavigationSourceHandler is public so I kept it to avoid breaking changes.

Before

image

After

image

ODataPathNavigationSourceHandler handler = new ODataPathNavigationSourceHandler();
path.WalkWith(handler);
return handler.NavigationSource;
for (int i = path.Count - 1; i > -1; --i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is i > -1; better or more performant than i >= 0?

Copy link
Contributor Author

@habbes habbes Jan 23, 2024

Choose a reason for hiding this comment

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

Good question. I'd assume it depends on the architecture. I may have naively assumed >= does "more" work than plan >, however, I think in this case architectures usually have special treatment for 0 and that may be more important here.

I have compared the assembly output of >= 0 and > -1 using sharplab.io and it appears that both of them get translated to the same assembly code which uses a greater than 0 comparison, rather than -1.

cmp dword ptr [ebp-8], 0 // comparison with 0
setge cl   // set if greater or equal

With that in mind, I'll change to >= 0 since it reads more naturally and it appears to be the compiler's preference.

gathogojr
gathogojr previously approved these changes Jan 23, 2024
@gathogojr gathogojr merged commit e28c230 into OData:main Jan 23, 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.

Perf: Optimize GetNavigationSource(ODataPath)
3 participants