Repository navigation
Reorder typeof checks to avoid infinite loops on StructrefProxy __hash__ - #8241
Conversation
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch. The proposed change seems like a good idea in general, typeof does indeed get called a lot and CFFI use is probably less likely than something with a _numba_type_ attribute! I think adding a test would be a good idea as it would help prevent potential future changes causing the same problem.
I presume the issue you saw was something along these lines:
from numba import njit, typeof
from numba.core import types
from numba.experimental import structref
@structref.register
class MyStructType(types.StructRef):
def preprocess_fields(self, fields):
return tuple()
class MyStruct(structref.StructRefProxy):
def __new__(cls):
return structref.StructRefProxy.__new__(cls)
def __hash__(self):
return MyStructHashImpl(self)
@njit
def MyStructHashImpl(self):
return 10
structref.define_proxy(MyStruct, MyStructType, [])
struct_inst = MyStruct()
typeof(struct_inst)which overflows the stack due to recursion, if so, perhaps this could be used as a basis for a test?
Many thanks.
stuartarchibald
added
4 - Waiting on author
sklam
left a comment
sklam
left a comment
There was a problem hiding this comment.
Just need a test as @stuartarchibald suggested and the small style nitpicking below
|
Also needs a quick flake8 fix. |
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch, two minor suggestions/fixes else looks good.
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch and fixes.
stuartarchibald
added
4 - Waiting on CI
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
stuartarchibald
added
5 - Ready to merge
|
Thank you for the patch! |
I was having an issue where I couldn't implement
__hash__for a StructRefProxy by calling into a dispatcher that takes the structref as an argument because the dispatcher calls typeof on the proxy instance which in turn calls cffi_utils.is_cffi_func() which calls__hash__on the instance creating an infinite loop.I'm not sure how often typeof is called (I suspect a lot), but my guess is that putting the
_numba_type_check first might provide a small compile-time performance benefit. I would imagine objects with_numba_type_are more common than cffi functions and thus should be checked first.I don't have a ton of time, but I'm happy to add a test if ya'll think it would be worth it.