[Cython] weird code in argument unpacking (memoryview related?)
Hi,
this code in Nodes.py around line 3950, at the end of the DefNodeWrapper's
method generate_keyword_unpacking_code(), was added as part of the memory
view changes, back in July last year:
"""
# convert arg values to their final type and assign them
for i, arg in enumerate(all_args):
if arg.default and not arg.type.is_pyobject:
code.putln("if (values[%d]) {" % i)
if arg.default and not arg.type.is_pyobject:
code.putln('} else {')
code.putln(
"%s = %s;" % (
arg.entry.cname,
arg.calculate_default_value_code(code)))
if arg.type.is_memoryviewslice:
code.put_incref_memoryviewslice(arg.entry.cname,
have_gil=True)
code.putln('}')
"""
By being overly complicated, it hides rather well what it's actually meant
to achieve. It doesn't do at all what its comment says and also refers to
the default value of the argument, which is already assigned way before
this place in the code. Therefore, I'd be surprised if it generated
anything but dead C code, because the case that "values[i]" is NULL should
never happen.
Mark, could you comment on this? Is this just a left-over from a broken
merge or something?
Stefan
___
cython-devel mailing list
cython-devel@python.org
http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] weird code in argument unpacking (memoryview related?)
On 30 March 2012 12:19, Stefan Behnel wrote:
> Hi,
>
> this code in Nodes.py around line 3950, at the end of the DefNodeWrapper's
> method generate_keyword_unpacking_code(), was added as part of the memory
> view changes, back in July last year:
>
> """
> # convert arg values to their final type and assign them
> for i, arg in enumerate(all_args):
> if arg.default and not arg.type.is_pyobject:
> code.putln("if (values[%d]) {" % i)
> if arg.default and not arg.type.is_pyobject:
> code.putln('} else {')
> code.putln(
> "%s = %s;" % (
> arg.entry.cname,
> arg.calculate_default_value_code(code)))
> if arg.type.is_memoryviewslice:
> code.put_incref_memoryviewslice(arg.entry.cname,
> have_gil=True)
> code.putln('}')
> """
>
> By being overly complicated, it hides rather well what it's actually meant
> to achieve. It doesn't do at all what its comment says and also refers to
> the default value of the argument, which is already assigned way before
> this place in the code. Therefore, I'd be surprised if it generated
> anything but dead C code, because the case that "values[i]" is NULL should
> never happen.
>
> Mark, could you comment on this? Is this just a left-over from a broken
> merge or something?
Hm, IIRC I was fixing acquisition count errors for memoryview slices
(of which there were many) for arguments with default values, I was
cleaning up after the initial memoryview implementation.
All I did was add another case for memoryview slices in old code (I
only wrote the last 3 lines, you wrote the other ones in 2008). Very
similar but more recent code (probably copied and pasted) can be found
around line ~3730 in generate_tuple_and_keyword_parsing_code (instead
of generate_keyword_unpacking_code).
There are tests for default arguments and acquisition count errors
should fail immediately and hard, so I trust your judgement to remove
any or all of this.
> Stefan
> ___
> cython-devel mailing list
> cython-devel@python.org
> http://mail.python.org/mailman/listinfo/cython-devel
___
cython-devel mailing list
cython-devel@python.org
http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] weird code in argument unpacking (memoryview related?)
mark florisson, 30.03.2012 14:25:
> On 30 March 2012 12:19, Stefan Behnel wrote:
>> this code in Nodes.py around line 3950, at the end of the DefNodeWrapper's
>> method generate_keyword_unpacking_code(), was added as part of the memory
>> view changes, back in July last year:
>>
>> """
>># convert arg values to their final type and assign them
>>for i, arg in enumerate(all_args):
>>if arg.default and not arg.type.is_pyobject:
>>code.putln("if (values[%d]) {" % i)
>>if arg.default and not arg.type.is_pyobject:
>>code.putln('} else {')
>>code.putln(
>>"%s = %s;" % (
>>arg.entry.cname,
>>arg.calculate_default_value_code(code)))
>>if arg.type.is_memoryviewslice:
>>code.put_incref_memoryviewslice(arg.entry.cname,
>>have_gil=True)
>>code.putln('}')
>> """
>>
>> By being overly complicated, it hides rather well what it's actually meant
>> to achieve. It doesn't do at all what its comment says and also refers to
>> the default value of the argument, which is already assigned way before
>> this place in the code. Therefore, I'd be surprised if it generated
>> anything but dead C code, because the case that "values[i]" is NULL should
>> never happen.
>>
>> Mark, could you comment on this? Is this just a left-over from a broken
>> merge or something?
>
> Hm, IIRC I was fixing acquisition count errors for memoryview slices
> (of which there were many) for arguments with default values, I was
> cleaning up after the initial memoryview implementation.
>
> All I did was add another case for memoryview slices in old code (I
> only wrote the last 3 lines, you wrote the other ones in 2008).
Well, the entire block appeared in your commit (and it looked suspiciously
like my own code in retrospect), so I assumed that you copied it from
somewhere or that it accidentally slipped in while handling a merge conflict.
> Very
> similar but more recent code (probably copied and pasted) can be found
> around line ~3730 in generate_tuple_and_keyword_parsing_code (instead
> of generate_keyword_unpacking_code).
Right - that one makes sense. The "values[i] == NULL" case is actually used
for non-Python default values. I'll add a comment.
The first method calls the latter, so the correct code is being executed
after the block in question. That makes it look like the one above is just
a redundant quirk. The fact that it went undetected should hint at a
missing test or missing refnanny usage, though. A duplicated incref would
imply a reference leak, and the fact that it only affects default arguments
would make it very hard to detect. Might be visible by using memory views
as default arguments in closures, now that Vitja's fix correctly handles those.
> There are tests for default arguments and acquisition count errors
> should fail immediately and hard, so I trust your judgement to remove
> any or all of this.
Given that the correct code is in the right place, I've deleted the copy. I
did it in a branch because I was rewriting a part of that code anyway, so
any attempt to fix it directly in the master would just unnecessarily lead
to collisions. The whole code is here, commit #6 being the cleanup:
https://github.com/cython/cython/pull/107
Stefan
___
cython-devel mailing list
cython-devel@python.org
http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] 0.16 release
On 27 March 2012 12:58, Stefan Behnel wrote: > mark florisson, 27.03.2012 13:20: >> I tested the release in my own branch and jenkins was blue, but >> the release build seems to disagree. > > The release (and master) branch is tested against the "-ext" builds of > CPython, which have some external packages installed, including NumPy. You > have to change the config of your "tests" job to use those instead of the > plain CPython builds. The build job can stay as it is. > > As for the inner workings, there's a normal "pyXY-hg" job to build CPython > and a corresponding "pyXY-ext-hg" job that takes the build and installs a > list of packages into it, then creates a new install archive. You can then > reference either of the two archives in your build/test jobs by asking for > a "pyXY" or "pyXY-ext" Python. > > >> The py32 C++ build shows some refcount error: >> >> numpy_memoryview.cpp:16922: warning: ‘void >> __pyx_pw_5numpy_7ndarray_3__releasebuffer__(PyObject*, Py_buffer*)’ >> defined but not used > > These are legitimate warnings that are worth fixing (at some point), I > think. They seem to originate from the buffer implementation in numpy.pxd. > Those sort-of-external special methods shouldn't lead to the generation of > a Python wrapper function. > > >> python: Modules/gcmodule.c:327: visit_decref: Assertion >> `gc->gc.gc_refs != 0' failed. > > It's surprising that that only occurs in one out of four Py3 test > configurations. Maybe there's something indeterministic in those tests? > > >> and some of the other python 3 tests are also failing. > > Yep, some of them look really funny (complaining about getting exactly the > expected output), others are the typical Py3 problems (e.g. printing bytes). > > Note that the reason the py3k tests are not impacted is that it does not > have NumPy. So the tests would equally fail in all Py3 versions. > > Stefan > ___ > cython-devel mailing list > cython-devel@python.org > http://mail.python.org/mailman/listinfo/cython-devel The release build looks good now, I'm thinking of pushing a second and final beta out there tomorrow. If anyone wants to get something in, now is the time to raise voice. ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] 0.16 release
On Fri, Mar 30, 2012 at 10:24 AM, mark florisson wrote: > The release build looks good now, I'm thinking of pushing a second and > final beta out there tomorrow. If anyone wants to get something in, > now is the time to raise voice. Excellent. I personally don't know of anything that can't wait 'till 0.16.1. If you're confident enough, let's call it a release candidate :). - Robert ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] 0.16 release
2012/3/30 Robert Bradshaw : > On Fri, Mar 30, 2012 at 10:24 AM, mark florisson > wrote: > >> The release build looks good now, I'm thinking of pushing a second and >> final beta out there tomorrow. If anyone wants to get something in, >> now is the time to raise voice. > > Excellent. I personally don't know of anything that can't wait 'till > 0.16.1. If you're confident enough, let's call it a release candidate > :). > I hope we're done with regressions now. I'm +1 for RC and feature freeze. -- vitja. ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
