cPyAMF

[thijs]

There are two parts to PyAMF: the core AMF encoder and decoder, and the gateway/remoting for transporting data between a server and a client.

Currently both parts are written in pure Python but the idea is to create a C version of the AMF encoder/decoder that can be used as a dropin which will increase performance significantly.

[nick]

My plan is to trial out Pyrex first so that we don't have to maintain a mess of c code.

The first step will be to check if there is anything within pyamf.amf* modules that shouldn't be there and move them to a more appropriate place. Then we can start building the .pyx files to generate the c.

[nick]

Branching to source:pyamf/branches/cpyamf-225 in r1109.

[nick]

Cython file for encoding variable length 29bit integers

[nick]

Generated c source file

[nick]

python benchmark script

[nick]

As a small experiment, I converted amf3._encode_int to a [Cython http://cython.org] file. These are the results i am getting from the benchmark:

         1032068 function calls in 3.619 CPU seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    3.619    3.619 <string>:1(<module>)
   262144    2.145    0.000    3.174    0.000 amf3.py:1718(_encode_int)
        1    0.436    0.436    3.619    3.619 test.py:7(r)
   769920    1.029    0.000    1.029    0.000 {chr}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
        1    0.009    0.009    0.009    0.009 {range}


         262148 function calls in 0.719 CPU seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.719    0.719 <string>:1(<module>)
        1    0.363    0.363    0.719    0.719 test.py:7(r)
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
   262144    0.349    0.000    0.349    0.000 {numbers.encode_int}
        1    0.007    0.007    0.007    0.007 {range}

The first listing is the pure python implementation and the second is the c extension. In just this small area, speed of encoding ints has been increased 7 fold.

[thijs]

Bumping milestone

[gerard]

I am looking to improve the performance of my PyAMF-based app.

Beyond the experiement attached to this ticket, has there been any further work done towards implementing cPyAMF?

If no progress has been made, I'm willing to donate development resources towards getting this done.

[nick]

We would love to get this ticket moving again!

I haven't done any further work towards it, but I think Arnar said he was tinkering ..

I'm on IRC (nick = njoyce) most of the time, stop by and we can chat about moving this forward.

[arnar]

I tinkered a little, but time escaped me yet again. IMO we should look into just writing a CPython module by hand, instead of Cython or such tools, it is not too hard.

Gerard, please go ahead and submit a patch if you can - there is no-one else working on it at the moment.

[gerard]

Okay, I will try to produce a patch. Should I go with CPython or Cython? Both sound manageable -- what would be most maintainble for you guys?

Also, what module(s) would you suggest I start from? pyamf.util looks like the right place to me.

[nick]

I am not much of a c dev so if its developed in pure c then I wouldn't be able to maintain it very well. On the other hand, we may not squeeze out all the performance if using Cython.

A personal preference would be to develop the extension in Cython, but if you are willing to support the extension long term then go right ahead.

I think that pyamf.util would be the right place to start.

[arnar]

Okay, I will try to produce a patch. Should I go with CPython or Cython?

I think you should choose whichever you think is better for you. My reasons for disliking Cython is the extra build dependency. I've also been burned in the past that sticking to a framework will get you cornered later. CPython extensions are fairly simple and don't require much C-coding experience. setup.py even takes care of building them for you.

I also understand Nick's qualms about maintenance, but in the long run I think knowing the intricacies of a system like Cython can end up as more work than maintaining a pure-C extension. I will also be around, so I can take the edge of bug fixes etc.

The operations that I think will benefit most from moving to C are the ones that do all the bit operations and byte packing, i.e. pyamf.util should cover most of it. I'd recommend starting by setting up a test-bed where you can easily run profiling tests and go from there.

[thijs]

Guys, just found out that someone started the libamf project (BSD licensed) very recently according to this comment. Would it be a good idea to try to hook PyAMF up to this project, instead of writing our own? I guess in the best scenario cPyAMF would be a Python wrapper for libamf?

[arnar]

I guess in the best scenario cPyAMF would be a Python wrapper for libamf?

That really depends on the design of libamf. PyAMF is already a fully fledged library with its own design choices. If libamf is easy to intergrate into that, sure. Without having looked at it I'd suspect a simple C version of key functions of our own library might be more light-weight.

By all means investigate.

[gerard]

From that comment:

I haven’t committed code yet, but I think I’ll have a beta available for testing by the end of august.

Considering that libamf doesn't exist yet, and that I'd like to have something working soon, I'll continue as planned.

[gerard]

First attempt at cpyamf

[gerard]

Okay, so I've attached my first stab at it.

Review and comments would be much appreciated.

[thijs]

Replying to gerard:

Okay, so I've attached my first stab at it. Review and comments would be much appreciated.

Thanks Gerard! Question; against which revision of PyAMF did you create the attached diff? When I try it against the latest (r1610) patch returns some (minor) issues:

(Stripping trailing CRs from patch.)
patching file cpyamf/util.h
(Stripping trailing CRs from patch.)
patching file cpyamf/util.c
(Stripping trailing CRs from patch.)
patching file setup.py
(Stripping trailing CRs from patch.)
patching file pyamf/tests/test_util.py
(Stripping trailing CRs from patch.)
patching file pyamf/tests/test_amf3.py
Hunk #1 succeeded at 1278 (offset 43 lines).
(Stripping trailing CRs from patch.)
patching file pyamf/util/__init__.py
Hunk #1 succeeded at 175 (offset 2 lines).
Hunk #2 succeeded at 190 (offset 2 lines).
Hunk #3 succeeded at 205 (offset 2 lines).
Hunk #4 succeeded at 220 (offset 2 lines).
Hunk #5 succeeded at 235 (offset 2 lines).
Hunk #6 succeeded at 250 (offset 2 lines).
Hunk #7 succeeded at 674 (offset 82 lines).

Also, are you registered on the dev MailingLists?? I'd love to involve others on the mailinglist with this review.

[thijs]

Attaching the build log against r1610 (shows 2 warnings, not sure if they're critical)

[thijs]

Added a build log on a Mac OSX 10.5.4 machine with MacPython? 2.5.2. What kind of OS/Python are you developing on Gerard?

[gerard]

Okay, I've checked in the code on a branch - /pyamf/branches/gerard-cpyamf-225

For reference, I'm noticing roughly a 35% improvement on calls to amf0.Encode(). It isn't a huge margin, but it's definitely a solid start.

These three compiler warnings are normal:

cpyamf/util.c:1222: warning: initialization from incompatible pointer type
cpyamf/util.c: In function `initutil':
cpyamf/util.c:1255: warning: dereferencing type-punned pointer will break strict-aliasing rules
cpyamf/util.c: At top level:
cpyamf/util.h:31: warning: 'cPyAmf_BufferedByteStream' defined but not used

[thijs]

Removing reference to Cython from ticket description.

[thijs]

Replying to thijs:

Also, are you registered on the dev MailingList? I'd love to involve others on the mailinglist with this review.

Subscribed you to the dev list, and asked for a review in case there are other C devs on the dev and users lists that can help out.

[thijs]

Could you also update your details in MAINTAINERS.txt that I added in r1615?

[nick]

Checks with valgrind, the first is running through cpyamf and the second is run on pure python:

==7590== ERROR SUMMARY: 1290 errors from 101 contexts (suppressed: 44 from 2)
==7590== malloc/free: in use at exit: 1,569,656 bytes in 347 blocks.
==7590== malloc/free: 15,416 allocs, 15,069 frees, 7,621,637 bytes allocated.
==7590== For counts of detected errors, rerun with: -v
==7590== searching for pointers to 347 not-freed blocks.
==7590== checked 1,573,028 bytes.
==7590== 
==7590== LEAK SUMMARY:
==7590==    definitely lost: 0 bytes in 0 blocks.
==7590==      possibly lost: 14,428 bytes in 39 blocks.
==7590==    still reachable: 1,555,228 bytes in 308 blocks.
==7590==         suppressed: 0 bytes in 0 blocks.
==7590== Rerun with --leak-check=full to see details of leaked memory.

==7594== ERROR SUMMARY: 6546 errors from 167 contexts (suppressed: 159 from 2)
==7594== malloc/free: in use at exit: 4,473,465 bytes in 2,881 blocks.
==7594== malloc/free: 67,150 allocs, 64,269 frees, 53,221,362 bytes allocated.
==7594== For counts of detected errors, rerun with: -v
==7594== searching for pointers to 2,881 not-freed blocks.
==7594== checked 4,539,664 bytes.
==7594== 
==7594== LEAK SUMMARY:
==7594==    definitely lost: 0 bytes in 0 blocks.
==7594==      possibly lost: 58,424 bytes in 159 blocks.
==7594==    still reachable: 4,415,041 bytes in 2,722 blocks.
==7594==         suppressed: 0 bytes in 0 blocks.
==7594== Rerun with --leak-check=full to see details of leaked memory.

[gerard]

Okay, everything appears to be working just fine on Python 2.3-2.5, on both 32 and 64 bit platforms. I don't have any plans on moving more classes over to C just yet, so for now I'll wait for feedback on what's already done.

Replying to thijs:

Could you also update your details in MAINTAINERS.txt that I added in r1615?

Done.

Replying to nick:

Checks with valgrind, the first is running through cpyamf and the second is run on pure python:

I've never used Valgrind, but I'm assuming that this is a good sign?

[thijs]

• is cpyamf now by default enabled or disabled? Or does it do some sort of compiler check?
• our repository currently has a separate folder for cpyamf, but maybe it's a better idea to move it into the main trunk, the way you implemented in the branch. thoughts?
• is cpyamf now being ignored for the jython platform?
• you should probably add your name to LICENSE.txt as well

I'll assign it to Arnar for review, regarding your valgrind question and a general review of the code.

Thanks!

[thijs]

Also looks like there are some docstrings missing in the PyMethodDef BufferedByteStream_methods of util.c

[thijs]

Replying to thijs:

- our repository currently has a separate folder for cpyamf, but maybe it's a better idea to move it into the main trunk, the way you implemented in the branch. thoughts?

I removed the old 'cpyamf' subproject from the repository in r1650, because the code should go in the main pyamf trunk.

[arnar]

I looked at the code, not thoroughly, but it all seems good to me. I don't have the facilities right here to test setup.py on different platforms (incl. non-cpython ones) which is something that needs to be done. The Jython case seems to be handled, wonder if something similar is needed for IronPython as well.

Gerard: One thing stood out, sometimes you use PycStringIO->bla(this->buffer, ...) and sometimes PyObject_CallMethod(this->buffer, ...). I don't know the PycStringIO api so maybe this is perfectly normal?

Otherwise, the c-code looks good. IMO the next thing that needs to happen is profiling of the encoding and decoding of some diverse bulk of data. Concrete profiling data should give us an idea if we should move the amf0 and amf3 encoders and/or decoders to C as well.

[thijs]

I'll test it with EchoTest and try the AMF dumps.

[thijs]

One of the unit tests failed:

===============================================================================
[ERROR]: test_amf3.EncoderTestCase.test_byte_array

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/unittest.py", line 260, in run
    testMethod()
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/tests/test_amf3.py", line 479, in test_byte_array
    self._run([(amf3.ByteArray('hello'), '\x0c\x0bhello')])
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/tests/test_amf3.py", line 298, in _run
    e.run(self)
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/tests/util.py", line 65, in run
    self.encoder.writeElement(n)
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/amf3.py", line 1241, in writeElement
    func = self._writeElementFunc(data)
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/__init__.py", line 613, in _writeElementFunc
    self._write_elem_func_cache[key] =  self._getWriteElementFunc(data)
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/__init__.py", line 584, in _getWriteElementFunc
    if callable(type_) and type_(data):
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/adapters/_django_db_models_fields.py", line 18, in <lambda>
    pyamf.add_type(lambda x: x == fields.NOT_PROVIDED, convert_NOT_PROVIDED)
  File "/Users/thijstriemstra/Sites/pyamf/pyamf/branches/gerard-cpyamf-225/pyamf/amf3.py", line 509, in __cmp__
    return cmp(self._buffer, other)
exceptions.AttributeError: 'ByteArray' object has no attribute '_buffer'
-------------------------------------------------------------------------------

[thijs]

• EchoTest completed without a problem.
• Also no problems with the AMF dumps

[gerard]

Replying to thijs:

- is cpyamf now by default enabled or disabled? Or does it do some sort of compiler check?

Right now, it builds by default. See /pyamf/branches/gerard-cpyamf-225/setup.py. Users can disable cpyamf by changing the appropriate variable in setup.py.

The way it works is that it checks for cpyamf at runtime when the pyamf.util module is loaded. If cpyamf is available it is used, otherwise, it uses the normal Python code.

- our repository currently has a separate folder for cpyamf, but maybe it's a better idea to move it into the main trunk, the way you implemented in the branch. thoughts?

Yes, it makes sense to put everything in one branch.

- is cpyamf now being ignored for the jython platform?

Yes. There's a basic check in setup.py which disables cpyamf on jython.

- you should probably add your name to LICENSE.txt as well

Will do. I also need to add proper headers to the files I've added.

[gerard]

Replying to arnar:

I looked at the code, not thoroughly, but it all seems good to me. I don't have the facilities right here to test setup.py on different platforms (incl. non-cpython ones) which is something that needs to be done. The Jython case seems to be handled, wonder if something similar is needed for IronPython as well.

Yes, something would probably be need for IronPython. I don't have all of those platforms available, though.

Gerard: One thing stood out, sometimes you use PycStringIO->bla(this->buffer, ...) and sometimes PyObject_CallMethod(this->buffer, ...). I don't know the PycStringIO api so maybe this is perfectly normal?

The C cStringIO API is very limited. They only give you a few methods (cwrite, cread, creadline, etc.). Rather than trying to get everything to work via those limited methods, I thought it'd make more sense to use the more direct calls available through Python.

Otherwise, the c-code looks good. IMO the next thing that needs to happen is profiling of the encoding and decoding of some diverse bulk of data. Concrete profiling data should give us an idea if we should move the amf0 and amf3 encoders and/or decoders to C as well.

My tests show that there would be some benefit of doing so. Unfortunately, I won't have the time to do so for a bit. For the time being, I'm happy with the ~35% gain from moving BufferedByteStream over.

[thijs]

Replying to gerard:

Replying to arnar:

I looked at the code, not thoroughly, but it all seems good to me. I don't have the facilities right here to test setup.py on different platforms (incl. non-cpython ones) which is something that needs to be done. The Jython case seems to be handled, wonder if something similar is needed for IronPython as well.

Yes, something would probably be need for IronPython. I don't have all of those platforms available, though.

Unfortunately we don't have a win32 buildslave available for testing things like IronPython so we'll have to do this sometime in the future. See #276 for the IronPython ticket.

Gerard, I think if you solve the last failing ByteArray unit test, I think we're ready to merge it.