[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[ https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414791#comment-16414791 ] ASF GitHub Bot commented on ARROW-2354: --- cpcloud commented on issue #1794: ARROW-2354: [C++] Make PyDecimal_Check() faster URL: https://github.com/apache/arrow/pull/1794#issuecomment-376349245 @pitrou @wesm Follow-up JIRA'd: https://issues.apache.org/jira/browse/ARROW-2357 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > [C++] PyDecimal_Check() is much too slow > > > Key: ARROW-2354 > URL: https://issues.apache.org/jira/browse/ARROW-2354 > Project: Apache Arrow > Issue Type: Bug > Components: C++, Python >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Assignee: Antoine Pitrou >Priority: Major > Labels: pull-request-available > Fix For: 0.10.0 > > > See https://github.com/apache/arrow/issues/1792 -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414780#comment-16414780
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud closed pull request #1794: ARROW-2354: [C++] Make PyDecimal_Check()
faster
URL: https://github.com/apache/arrow/pull/1794
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/cpp/src/arrow/python/helpers.cc b/cpp/src/arrow/python/helpers.cc
index 5719af6f3..63fee54b6 100644
--- a/cpp/src/arrow/python/helpers.cc
+++ b/cpp/src/arrow/python/helpers.cc
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out) {
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
+ if (!decimal_type.obj()) {
+Status status = ImportDecimalType(_type);
+DCHECK_OK(status);
+DCHECK(PyType_Check(decimal_type.obj()));
+ }
+ // PyObject_IsInstance() is slower as it has to check for virtual subclasses
+ const int result =
+ PyType_IsSubtype(Py_TYPE(obj),
reinterpret_cast(decimal_type.obj()));
+ DCHECK_NE(result, -1) << " error during PyType_IsSubtype check";
return result == 1;
}
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
> Fix For: 0.10.0
>
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414662#comment-16414662
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177253437
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
+ if (!decimal_type.obj()) {
Review comment:
@wesm This is my only comment, which isn't necessary for merging.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[ https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414657#comment-16414657 ] ASF GitHub Bot commented on ARROW-2354: --- wesm commented on issue #1794: ARROW-2354: [C++] Make PyDecimal_Check() faster URL: https://github.com/apache/arrow/pull/1794#issuecomment-376330717 @cpcloud I'll wait for you to have a last look at this before merging (you have a comment unaddressed still) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > [C++] PyDecimal_Check() is much too slow > > > Key: ARROW-2354 > URL: https://issues.apache.org/jira/browse/ARROW-2354 > Project: Apache Arrow > Issue Type: Bug > Components: C++, Python >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Assignee: Antoine Pitrou >Priority: Major > Labels: pull-request-available > > See https://github.com/apache/arrow/issues/1792 -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414495#comment-16414495
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177230296
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
Great.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[ https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414224#comment-16414224 ] ASF GitHub Bot commented on ARROW-2354: --- pitrou commented on issue #1794: ARROW-2354: [C++] Make PyDecimal_Check() faster URL: https://github.com/apache/arrow/pull/1794#issuecomment-376254213 The benchmark already exists, see above :-) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > [C++] PyDecimal_Check() is much too slow > > > Key: ARROW-2354 > URL: https://issues.apache.org/jira/browse/ARROW-2354 > Project: Apache Arrow > Issue Type: Bug > Components: C++, Python >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Assignee: Antoine Pitrou >Priority: Major > Labels: pull-request-available > > See https://github.com/apache/arrow/issues/1792 -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[ https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414222#comment-16414222 ] ASF GitHub Bot commented on ARROW-2354: --- wesm commented on issue #1794: ARROW-2354: [C++] Make PyDecimal_Check() faster URL: https://github.com/apache/arrow/pull/1794#issuecomment-376253924 Can you add an ASV benchmark to exercise the performance issue in https://github.com/apache/arrow/issues/1792? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > [C++] PyDecimal_Check() is much too slow > > > Key: ARROW-2354 > URL: https://issues.apache.org/jira/browse/ARROW-2354 > Project: Apache Arrow > Issue Type: Bug > Components: C++, Python >Affects Versions: 0.9.0 >Reporter: Antoine Pitrou >Assignee: Antoine Pitrou >Priority: Major > Labels: pull-request-available > > See https://github.com/apache/arrow/issues/1792 -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414184#comment-16414184
]
ASF GitHub Bot commented on ARROW-2354:
---
pitrou commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177172524
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
If the first implementation fails for someone, they'll notice and tell us.
If the second implementation fails for someone, it will produce mysterious
slowdowns.
So I think the first implementation should win, for now at least.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414182#comment-16414182
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177171726
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
I guess it's a question of which is more stable: the C implementation of the
decimal module or the way Python names types?
I personally don't feel strongly about either one. If I was forced to
choose, I probably stick with the first implementation. It seems less brittle,
because it doesn't depend on Python's naming convention, but that intuition
could be misleading. Maybe that's a very stable part of Python.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414170#comment-16414170
]
ASF GitHub Bot commented on ARROW-2354:
---
pitrou commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177169733
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
Here is an alternative implementation. Which one do you prefer?
```c++
bool PyDecimal_Check(PyObject* obj) {
OwnedRef decimal_type;
PyTypeObject* obj_type = Py_TYPE(obj);
// Fast fail for most types other than Decimal, for speed
if (strcmp(obj_type->tp_name, "decimal.Decimal")) {
return false;
}
Status status = ImportDecimalType(_type);
DCHECK_OK(status);
DCHECK(PyType_Check(decimal_type.obj()));
// PyObject_IsInstance() is slower as it has to check for virtual
subclasses
const int result =
PyType_IsSubtype(obj_type,
reinterpret_cast(decimal_type.obj()));
DCHECK_NE(result, -1) << " error during PyType_IsSubtype check";
return result == 1;
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414166#comment-16414166
]
ASF GitHub Bot commented on ARROW-2354:
---
pitrou commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177168902
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
No, because depending on how the C `decimal` module is implemented, there
may be a separate Decimal type per interpreter. This is currently not the case,
though, so perhaps we shouldn't worry about it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414153#comment-16414153
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177167278
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
Is that because the reference count will be shared across them?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414151#comment-16414151
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177166321
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
+ if (!decimal_type.obj()) {
+Status status = ImportDecimalType(_type);
+DCHECK_OK(status);
+DCHECK(PyType_Check(decimal_type.obj()));
+ }
+ // PyObject_IsInstance() is slower as it has to check for virtual subclasses
Review comment:
This was the cause of the performance regression, and not importing over and
over? Or was it both?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414150#comment-16414150
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177166026
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
+ if (!decimal_type.obj()) {
Review comment:
Maybe compare against `nullptr` here?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414152#comment-16414152
]
ASF GitHub Bot commented on ARROW-2354:
---
pitrou commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177167130
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
+ if (!decimal_type.obj()) {
+Status status = ImportDecimalType(_type);
+DCHECK_OK(status);
+DCHECK(PyType_Check(decimal_type.obj()));
+ }
+ // PyObject_IsInstance() is slower as it has to check for virtual subclasses
Review comment:
It was importing over and over.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414143#comment-16414143
]
ASF GitHub Bot commented on ARROW-2354:
---
pitrou commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177166295
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
- OwnedRef Decimal;
- Status status = ImportDecimalType();
- DCHECK_OK(status);
- const int32_t result = PyObject_IsInstance(obj, Decimal.obj());
- DCHECK_NE(result, -1) << " error during PyObject_IsInstance check";
+ static OwnedRef decimal_type;
Review comment:
Note that this may not play well with multiple Python (sub)interpreters in
the same process.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (ARROW-2354) [C++] PyDecimal_Check() is much too slow
[
https://issues.apache.org/jira/browse/ARROW-2354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16414137#comment-16414137
]
ASF GitHub Bot commented on ARROW-2354:
---
cpcloud commented on a change in pull request #1794: ARROW-2354: [C++] Make
PyDecimal_Check() faster
URL: https://github.com/apache/arrow/pull/1794#discussion_r177165146
##
File path: cpp/src/arrow/python/helpers.cc
##
@@ -227,12 +227,16 @@ Status UInt64FromPythonInt(PyObject* obj, uint64_t* out)
{
}
bool PyDecimal_Check(PyObject* obj) {
- // TODO(phillipc): Is this expensive?
Review comment:
Guess the answer is "yes" :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> [C++] PyDecimal_Check() is much too slow
>
>
> Key: ARROW-2354
> URL: https://issues.apache.org/jira/browse/ARROW-2354
> Project: Apache Arrow
> Issue Type: Bug
> Components: C++, Python
>Affects Versions: 0.9.0
>Reporter: Antoine Pitrou
>Assignee: Antoine Pitrou
>Priority: Major
> Labels: pull-request-available
>
> See https://github.com/apache/arrow/issues/1792
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
