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

Can we use IsEqualGUID in the GUID.__eq__? #659

Open
junkmd opened this issue Nov 10, 2024 · 4 comments
Open

Can we use IsEqualGUID in the GUID.__eq__? #659

junkmd opened this issue Nov 10, 2024 · 4 comments
Labels
good first issue Good for newcomers

Comments

@junkmd
Copy link
Collaborator

junkmd commented Nov 10, 2024

The module defining GUID includes the following "Note":

# Note: Comparing GUID instances by comparing their buffers
# is slightly faster than using ole32.IsEqualGUID.

However, this note was written at the very start of this project, even before the release of Python 3.0.

CPython and ctypes have improved over the past years.
Is there a performance difference between the current implementation and an implementation with IsEqualGUID like the following?

@@ -13,6 +13,7 @@ def binary(obj: "GUID") -> bytes:
 BYTE = c_byte
 WORD = c_ushort
 DWORD = c_ulong
+TRUE = 1

 _ole32 = oledll.ole32

@@ -22,9 +23,7 @@ _ProgIDFromCLSID = _ole32.ProgIDFromCLSID
 _CLSIDFromString = _ole32.CLSIDFromString
 _CLSIDFromProgID = _ole32.CLSIDFromProgID
 _CoCreateGuid = _ole32.CoCreateGuid
-
-# Note: Comparing GUID instances by comparing their buffers
-# is slightly faster than using ole32.IsEqualGUID.
+_IsEqualGUID = _ole32.IsEqualGUID


 class GUID(Structure):
@@ -50,7 +49,7 @@ class GUID(Structure):
         return self != GUID_null

     def __eq__(self, other) -> bool:
-        return isinstance(other, GUID) and binary(self) == binary(other)
+        return isinstance(other, GUID) and _IsEqualGUID(byref(self), byref(other)) == TRUE

     def __hash__(self) -> int:
         # We make GUID instances hashable, although they are mutable.

After comparing benchmarks, if it proves effective, feel free to submit a PR.

@junkmd junkmd added good first issue Good for newcomers and removed good first issue Good for newcomers labels Nov 10, 2024
@junkmd junkmd changed the title Use IsEqualGUID in the GUID.__eq__. Can we use IsEqualGUID in the GUID.__eq__. Nov 10, 2024
@junkmd junkmd changed the title Can we use IsEqualGUID in the GUID.__eq__. Can we use IsEqualGUID in the GUID.__eq__? Nov 10, 2024
@jegg-dev
Copy link

jegg-dev commented Dec 9, 2024

New contributor here, I've benchmarked both implementations of GUID.__eq__ using the existing GUID unit tests (but modified to run each test repeatedly).

Buffer implementation:

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
400000    0.294    0.000    0.527    0.000 guid.py:55(__eq__)

IsEqualGUID Implementation:

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
400000    0.355    0.000    0.467    0.000 guid.py:55(__eq__)

There is a performance difference of ~12% (150 ns/exec, 60 ms total) in favor of IsEqualGUID for cumulative execution time. I've observed similar results on repeated runs.

@junkmd
Copy link
Collaborator Author

junkmd commented Dec 9, 2024

@jegg-dev

Thank you for your investigation.

Welcome to our community!

Which Python version did you test this on?

From your investigation, I understand that the Buffer implementation performs better when the number of calls is low, whereas the IsEqualGUID implementation becomes more advantageous when the number of calls is high.

How frequently is GUID.__eq__ called in this project?
This will determine whether we should replace the current implementation.

@jegg-dev
Copy link

jegg-dev commented Dec 9, 2024

Python 3.11.0 was used for testing.

I'm not exactly sure how to give a good answer for frequency of GUID.__eq__ calls but I ran the test suite (excluding test_GUID) and counted 49 calls. As for references to the function in the codebase, I was not able to find any explicit uses after manually searching for GUID references.

@junkmd
Copy link
Collaborator Author

junkmd commented Dec 12, 2024

@jegg-dev

Sorry for the delayed response.

A new issue, #696, has been opened regarding GUID.
We should address that first before proceeding with this performance comparison.

If you’re interested in contributing to the codebase of this project, it would be great if you could work on #696.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants