-
Notifications
You must be signed in to change notification settings - Fork 56
CVS-178131: perftest: iterate over the metadata entries first in filter_ep_devices #892
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
CVS-178131: perftest: iterate over the metadata entries first in filter_ep_devices #892
Conversation
e599630 to
c6c1d5d
Compare
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.
Pull request overview
This PR optimizes the device filtering logic in the perftest by reordering nested loops. The key change moves the iteration over metadata key-value pairs to the outer loop, with device iteration in the inner loop, which should improve performance when there are many devices but few filter criteria.
- Swapped loop order: metadata key-value pairs are now iterated in the outer loop instead of the inner loop
Comments suppressed due to low confidence (1)
onnxruntime/test/perftest/ort_test_session.cc:1
- The reordered loops now cause
GetKeyValuePairs()to be called repeatedly for the same device when there are multiple filter criteria. This could be less efficient than the original code ifGetKeyValuePairs()is expensive. Consider caching these values outside the outer loop or ensuring the original loop order is maintained if metadata retrieval has significant overhead.
// Copyright (c) Microsoft Corporation. All rights reserved.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto device = ep_devices[index]; | ||
| if (ep_set.find(std::string(device.EpName())) == ep_set.end()) | ||
| continue; | ||
| for (const auto& kv : performance_test_config.filter_ep_device_kv_pairs) { |
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.
Changes look good. I would only recommend updating the usage documentation for this option to explicitly define what the expected matching order should be.
Perhaps a simple example like:
--filter_ep_devices "ov_device|NPU ov_device|CPU" means that an NPU device, if available, will be added first before one with cpu.
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.
updated, also skipped if the device already added.
b8f475d to
20035c0
Compare
20035c0 to
adfee3d
Compare
MayureshV1
left a comment
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.
Looks good to merge !
Fix for AUTO device type, to honor the ordering of the passed devices.
CVS-178131