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

Fixes #2041: Improved performance of and cleaned up equality test for CIM objects #2092

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

andy-maier
Copy link
Contributor

@andy-maier andy-maier commented Jan 26, 2020

This is the second stage of addressing issue #2041, in which the actual performance improvement gets implemented.

Original equality performance:

  • CIMInstanceName with two keybindings, last one different: 14.077 us (stddev 5.2% in 100 runs with 14 outliers)
  • CIMClass with 10 properties, last one different: 197.023 us (stddev 3.0% in 100 runs with 11 outliers)

Improved equality performance:

  • CIMInstanceName with two keybindings, last one different: 12.952 us (stddev 4.6% in 100 runs with 11 outliers)
  • CIMClass with 10 properties, last one different: 173.040 us (stddev 3.5% in 100 runs with 7 outliers)

As a result, this change is not a significant performance improvement, but it did clean up the code and eliminated the old comparator-based logic.

Update related to Karl's suggestion in a review comment to merge 4 lines of conditional into an expression:

This makes the improvement slightly better. Note that there are still variations in the measured result:

With the original code (4 lines of conditional):

  • CIMInstanceName with two keybindings, last one different: 13.448 us (std.dev. 4.3% in 100 runs with 12 outliers)
  • CIMClass with 10 properties, last one different: 176.183 us (std.dev. 2.8% in 100 runs with 14 outliers)

Latest code in this PR (4 lines condensed into one expression):

  • CIMInstanceName with two keybindings, last one different: 13.703 us (std.dev. 6.1% in 100 runs with 12 outliers)
  • CIMClass with 10 properties, last one different: 182.927 us (std.dev. 3.2% in 100 runs with 17 outliers)

@andy-maier andy-maier changed the title Andy/eq efficiency Fixes #2041: Improved performance of and cleaned up equality test for CIM objects Jan 26, 2020
@andy-maier andy-maier self-assigned this Jan 26, 2020
@andy-maier andy-maier added this to the 1.0.0 milestone Jan 26, 2020
@coveralls
Copy link

coveralls commented Jan 26, 2020

Coverage Status

Coverage increased (+0.07%) to 87.161% when pulling 3aac528 on andy/eq-efficiency into 8f94687 on master.

@andy-maier andy-maier force-pushed the andy/eq-efficiency branch 2 times, most recently from d76d422 to 20269d6 Compare January 27, 2020 22:48
pywbem/_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

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

Just wondering if reduction a couple of places to single line might give python to optimize but in any case congrats on code improvement.

@andy-maier andy-maier force-pushed the andy/eq-efficiency branch 4 times, most recently from c1447f5 to 9ccb907 Compare January 28, 2020 13:38
Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
Details:

* Added testcases to CIMInstanceName that set keybindings to None
* Fixed that some equality test cases were defined but not run
* Added datetime equality testcases

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
Details:

* Migrated from comparator functions to equality functions. this
  includes adding new equality test functions _eq_dict/item/name() in the
  _utils.py module, and removing the cmpdict/cmpitem/cmpname() functions.

  This simplied the implementation so that a small performance improvement
  could be achieved.

* Fixed a float/int issue in test_perf_equality.py

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
@andy-maier andy-maier merged commit 624e611 into master Jan 28, 2020
@andy-maier andy-maier deleted the andy/eq-efficiency branch January 28, 2020 16:12
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.

3 participants