-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add HPy_CallTupleDict, HPyCallable_Check and HPyTuple_Check. #147
Conversation
Ready for review. |
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.
the PR looks good.
I'm not 100% sure that we can to call it HPy_Call
, though. On CPython it is called that way because it matches the signature of tp_call
, but in HPy we will probably end up with a different signature, so it will be a bit weird.
One option could be to call it HPy_LegacyCall
maybe? This would leave HPy_Call
free for whatever signature we think it's more appropriate later.
test/test_call.py
Outdated
def f(a, b): | ||
return a + b | ||
|
||
assert mod.call(f, (1, 2)) == 3 |
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.
maybe we should also test what happens if you pass something which is not a tuple and/or a dict?
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.
Good idea. I'll add some.
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.
The answer is that C Python segfaults in some cases and works fine in others (e.g. a list of args). I've added tests and changed the HPy implementation to check the types of handles rather than segfault.
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.
I wonder if doing these extra check costs any performance. Do you feel like adding microbenchmarks?
If they are costly, we could do the check only in debug mode.
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.
I'm guessing not a whole lot, but I'll add the microbenchmarks.
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.
Microbenchmarks added.
@antocuni I don't mind a different name, but:
I don't have another good name to suggest really, so I'm a bit stuck on this point. Any other suggestions? If it's not going to be the same name as "_Call", I think the name should try describe the method. |
I agree that no solutin is perfect, it's the usual tradeoff between keeping compatibility with the old API and designing a better one.
Your version of As for point 2, I claim we should discourage it :). In most cases creating a tuple just to call a function is inefficient and unnecessary: on PyPy, we just don't need the tuple at all and CPython is migrating toward vector call and thus more native functions support the faster calling convention. I don't know about GraalPython, maybe @fangerer or @timfel have opinions? Of course we should support a way to call things "the old way", that's why I proposed I tried to do a quick grep on the numpy source code to see how it is used in the wild. Excluding the cython-generated sources, there are 13 usages of |
For clarity, are you proposing to have the names be |
What about |
CPython implements So, let's implement only the equivalent of
I don't have any strong preference. |
Note that in the old C API, |
I like explicit, too. In general we would of course like to avoid creating the tuple for calls if we can. When not running in ABI mode (i.e., everything is compiled and run as bitcode and jitted on Graal) we can sometimes remove the tuple allocation if we're lucky, but a) that's more work for the optimizer and b) it's better if ABI mode doesn't incur a performance hit on GraalPython that would push users towards compiling per VM yet again. |
I've unified things into a single Thoughts? |
I think that having CallTupleDict which maps to either Call or CallObject is fine, provided that it doesn't cost any performance on CPython. |
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.
I like it :), see also my comments above.
Also, maybe the title of the PR should be updated
} | ||
else { | ||
// args is null, but kw is not, so we need to create an empty args tuple | ||
// for CPython's PyObject_Call |
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.
ah, nice catch. I didn't know that CPython doesn't have a way to pass only kwargs. I don't know if it will be actually useful but it doesn't cost anything to add support for it, so it's probably a good idea to be more complete, +1
test/test_call.py
Outdated
|
||
|
||
class TestCall(HPyTest): | ||
def argument_combinations(self, *items): |
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.
good idea! Maybe write it to take **items
instead of *items
? It would make the calls much nicer to read, and and I think that on modern Python the order of items in the dict is guaranteed, isn't it?
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.
I think it is. Let me try that.
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.
Woot. Implemented. Thank you for the suggestion.
Porting guide updated. |
@antocuni Ready for a (hopefully) final review. |
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.
Thank you, I think it's ready to merge!
For the records, I tried to run the microbench on bencher7, here are the results or a single run (I ran it few times and didn't see any noticeable difference):
cpy hpy
---------------- -------------------
TestModule::test_call_with_tuple 1144.95 us 1146.30 us [1.00]
TestModule::test_call_with_tuple_and_dict 1909.51 us 1884.72 us [0.99]
Thanks for reviewing! Your benchmark numbers look very similar to those on my laptop, where there is also no difference visible across several runs. |
Implements the simpler pythonic calling API for #122.