Skip to content

Conversation

@JobJob
Copy link
Contributor

@JobJob JobJob commented Apr 4, 2018

Accessing Python tuples with integer indices is common, e.g. for functions with multiple return values.

The ideas here are to add new methods get(o::PyObject, returntype::TypeTuple, k::Int) - and also to add an equivalent get!(ret::PyObject, ...) which assigns the result to ret, without creating a new PyObject.

They use :PyTuple_GetItem from the Python API, which takes an Int (py_ssize_t) rather than a PyObject*.

Benchmarks on my machine, for mean times (which are most relevant for accessing return values of functions in a tight loop, since they include gc cost) are:

Mean times
----------
standard get        : TrialEstimate(357.580 ns)
get!                : TrialEstimate(191.786 ns)
getwpyisinst!       : TrialEstimate(14.595 ns)
getwtuple_ptr!      : TrialEstimate(12.279 ns)
unsafe_gettpl!      : TrialEstimate(5.144 ns)

I think we should probably go with getwpyisinst!, and also add unsafe_gettpl! as a separate function (it can segfault if you use a bad index).


import Base: get!
function get!(ret::PyObject, o::PyObject, returntype::TypeTuple, k)
ret.o = @pycheckn ccall((@pysym :PyObject_GetItem),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to decref the old ret.o pointer if it is non-NULL. Probably write a copy!(o::PyObject, p::PyPtr) method to encapsulate this pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good pick up, I was wondering about that.

@JobJob JobJob force-pushed the fast-tuple-access branch from 1320f2a to 9f70333 Compare April 9, 2018 18:52
@JobJob JobJob changed the title RFC: Ideas for speeding up tuple access Speed up tuple/list access Apr 13, 2018
@stevengj
Copy link
Member

Looks good to me.

(Note that in the 0.7 version of PyCall, where I use getproperty, o.x in Julia will be equivalent to o.x in Python and o[x] in Julia will be equivalent to o[x] in Python, so I will stop doing the automatic adjustment from one-based to zero-based indexing, and instead define an appropriate firstindex etc. Not sure how I will manage the deprecation for that.)

@JobJob
Copy link
Contributor Author

JobJob commented Apr 15, 2018

Right, so o.s will be the equivalent of o[:s] currently?

getindex(o::PyObject, s::Symbol) = convert(PyAny, getindex(o, string(s)))
which ends up calling PyObject_GetAttrString with o and s

and for o[idx::Integer] you'll drop the ind2py calls in these?

PyCall.jl/src/PyCall.jl

Lines 777 to 786 in fb75b96

_getindex(o::PyObject, i::Integer, T) = convert(T, PyObject(@pycheckn ccall((@pysym :PySequence_GetItem), PyPtr, (PyPtr, Int), o, ind2py(i))))
getindex(o::PyObject, i::Integer) = _getindex(o, i, PyAny)
function setindex!(o::PyObject, v, i::Integer)
@pycheckz ccall((@pysym :PySequence_SetItem), Cint, (PyPtr, Int, PyPtr), o, ind2py(i), PyObject(v))
v
end
getindex(o::PyObject, i1::Integer, i2::Integer) = get(o, (ind2py(i1),ind2py(i2)))
setindex!(o::PyObject, v, i1::Integer, i2::Integer) = set!(o, (ind2py(i1),ind2py(i2)), v)
getindex(o::PyObject, I::Integer...) = get(o, map(ind2py, I))
setindex!(o::PyObject, v, I::Integer...) = set!(o, map(ind2py, I), v)

Seems ok, though I'm not really seeing why moving to getproperty necessitates switching to 0-based Integer indexing? I'd just been imagining getproperty would be sugar for PyObject_GetAttrString. Maybe you have a motivating example, and/or I'm missing something?

Anyway, some ideas for deprecation if you do go for it:

You could put one of these warnings on (edit: the first call in a session to) getindex(o,i::Integer)

  1. """PyCall has moved to 0-based indexing, to silence this warning set ENV["PYWARNZERO"] = "1". To continue using 1-based indexing, set ENV["PYONEBASED"]="1", this option will be dropped in v2.0 to ensure consistency across codebases""" or
  2. """PyCall will move to 0-based indexing in v2.0, to silence this warning set ENV["PYWARNZERO"] = "1" """

I think 1 is good only if a macro-based check for ENV["PYONEBASED"] would work with pre-compilation, otherwise 2 is prob better.

@stevengj
Copy link
Member

stevengj commented Apr 26, 2018

Seems ok, though I'm not really seeing why moving to getproperty necessitates switching to 0-based Integer indexing? I'd just been imagining getproperty would be sugar for PyObject_GetAttrString. Maybe you have a motivating example, and/or I'm missing something?

Because o.foo in Julia will be equivalent to o.foo in Python, and o[foo] in Julia will correspond to o[foo] in Python. Once that change is made, it will be weird to say o[foo] is the same as o[foo] in Python, except if foo is an integer, in which case we subtract 1.

Also, in Julia 0.7 there is better support for 0-based arrays in general with the new firstindex function.

@JobJob JobJob force-pushed the fast-tuple-access branch 2 times, most recently from b0fcd8e to d058e01 Compare April 30, 2018 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants