BigInteger support in PInt #346
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
...graal.python/src/com/oracle/graal/python/builtins/objects/foreign/ForeignObjectBuiltins.java
Outdated
Show resolved
Hide resolved
...graal.python/src/com/oracle/graal/python/builtins/objects/foreign/ForeignObjectBuiltins.java
Outdated
Show resolved
Hide resolved
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA). To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
OCA Required
JaroslavTulach
force-pushed
the
jtulach/BigIntegerSupport
branch
from
0043631 to
9af32ec
Compare
|
Thank you for signing the OCA. |
oracle-contributor-agreement
bot
added
OCA Verified
|
Is anything blocking integration of my PR? |
| @@ -1031,6 +1031,28 @@ protected static long doIt(Object object, | |||
| gil.acquire(); | |||
| } | |||
| } | |||
| @Specialization(limit = "3", guards="lib.fitsInBigInteger(object)") | |||
There was a problem hiding this comment.
If the value does not fit in long nor big integer, then this will cause UnsupportedSpecializationException, while previously it would cause Python level exception that the user can handle. Why another @Specialization and not another if in the existing specialization?
There was a problem hiding this comment.
Could we just add a @Fallback raising the Python exception?
There was a problem hiding this comment.
Yes, but I am not sure if this is correct because of what you pointed out in #346 (comment). I think that one would just need more complex example than the tests that were added to trigger the potential issue. All in all, adding an if to the single @Specialization looks like safe and simple solution.
There was a problem hiding this comment.
OK, merging the two @Specializations back into one: 04bf4c6 - thanks for your review, Štěpáne. There are two reasons why I went with a separate specialization:
- boxing return type
- need for additional node
Boxing isn't probably issue, it is going to happen (in the interpreter) later anyway. @Cached PythonObjectFactory factory is however unnecessary unless the value is really big.
There was a problem hiding this comment.
Out of curiosity - is there any (expected, of course in practice one would have to measure this) advantage (e.g. in performance) of having 2 separate simpler specializations VS one more complex one?
There was a problem hiding this comment.
Ah, I forgot about PythonObjectFactory that's good point. I'd still keep it this way even though it's a small trade off of readability vs efficiency. Regarding performance: the specialization will get new instance for each interop object "type" and each such instance will specialize to that specific "type", so I don't think that two specializations would gain anything in this case except for the removal of the unnecessary PythonObjectFactory allocation. OTOH, more specializations, especially with guards => more complex execute and executeAndSpecialize => can have impact on the interpreter performance (see also host inlining)
graalvmbot
closed this pull request by merging all changes
into
oracle:master
in
8edd56c
|
Thank you @ppisl for accepting, adopting and integrating my proposal. |
Expose big integers by reacting to
asBigIntegermessage inPInt. Making sure big integer can be passed into Python.