Deferred implementation in C

[therve]

For optimization (and fun) purpose I created a implementation of Deferred in C, inspired by the pyrex version lying around svn sandbox. I intentionally left out the debug and timeout stuff.

It seems to pass most Twisted suite, except those involving setTimeout and debug.

I'm putting this here for anyone interested. On some basic tests, my apps are 5 to 20% faster.

[exarkun]

This kind of investigation may be useful, but we need a performance benchmark suite for Deferreds before it can be productive, I think.

James Knight has previously implemented Deferred in Pyrex. The reason this isn't included in Twisted is that we just have no reliable, accurate way to measure what the performance changes of this are. The same applies to the attached cdefer.c.

In the interest of reducing the ever rising tide of tickets, I am closing this one, since it seems very unlikely to me that the work necessary to resolve it in a useful way is actually going to get done. Creating this as a low priority ticket was the right kind of idea, but what trac really needs is a whole separate category of things that might be interesting but are unlikely to happen.

If you (therve) or anyone else would like to contribute a benchmark which makes it possible to determine the utility of a Pyrex or C implementation of Deferred, please feel free to re-open this ticket: this is not a rejection of the idea of a more optimized Deferred implementation.

Deriving something from the code which showed a 5% - 20% speedup might be a good way to start on such a benchmark. It's important to cover all uses of Deferreds here. It's also okay if some parts show a slowdown, as long as the overall improvement is enough. Of course, it's better if everything is faster. :)

[therve]

Thanks a lot for your comments. A benchmark tool would be awesome, I'm putting this idea on my hope-i'll-have-some-time-one-day todo list. The main problem is to test real code, not some quick benchmark around Deferred.addCallback. It could even be a great buildbot metric of Twisted (for problem like #1524).

Until then I'll try to close some tickets :).

[Peaker]

After my review -- fixing a lot of error-handling/refcounting bugs

[Peaker]

* I added test cases for a bunch of bugs that existed. * I added benchmarks for deferred behaviours:

• With various N values (Which indeed discover that the old deferred has much worse-than-linear behaviour in various places, the C one is linear)
• Benchmarks have good coverage of the .py implementation (except for chainDeferred, which is uninteresting to benchmark, debug-stuff which do not exist in the C version, and setTimeout code which also is not implemented in cDeferred)

* Reviewed the C code and fixed a lot of error handling/refcounting bugs (All of them, hopefully). * Added the "Unhandled Error in Deferred" mechanism, so those are still reported in the same way (and the relevant tests pass, too) with cdefer.

Notes: * The C deferred finally dropped support for the deprecated setTimeout mechanism. * With the C deferred, setDebugging/getDebugging "works", but does not do anything. This fails a few debug-info tests (testAlreadyCalledDebug_[CE][CE]) that verify some debug output. * Unhandled Error in Deferred still works.

[Peaker]

The benchmark results on my machine

[Peaker]

Fixed a reentrancy bug (runCallbacks->callback->addCallbacks->runCallbacks). Added relevant test cases. Fixed use of defer.setTimeout in twisted.mail

[Peaker]

Fixed a reentrancy bug (runCallbacks->callback->addCallbacks->runCallbacks). Added relevant test cases. Fixed use of defer.setTimeout in twisted.mail

[exarkun]

I applied the patch to the branch cdefer-2245.

I haven't really looked at the code, but I did notice that the attached cdefer.c doesn't build against against Python 2.3 or Python 2.4.

[Peaker]

Use int instead of Py_ssize_t as a PyList index.

[therve]

I applied the patch on the branch and had a quick look. The modifications you made looks reasonable.

Unfortunately to be reviewed all the tests must pass, and for now the tests on debug and setTimeout doesn't pass. The latter could be skipped I guess since setTimeout is deprecated, but we have to find a solution to the former.

[Peaker]

Replying to therve:

I applied the patch on the branch and had a quick look. The modifications you made looks reasonable. Unfortunately to be reviewed all the tests must pass, and for now the tests on debug and setTimeout doesn't pass. The latter could be skipped I guess since setTimeout is deprecated, but we have to find a solution to the former.

I added a new patch that passes all of defer's tests:

I set setTimeout tests to only run if hasattr(Defer, "setTimeout") (I am not sure if this is better than actually removing the tests).

I changed setDebugging to only apply to newly created Deferreds, by using the Py-deferred when debugging is set to True, rather than the C deferred one. This allows for backwards compatibility with debug, the tests to pass, at the expense of performance in those cases.

[Peaker]

Fixes setDebugging/setTimeout tests

[Peaker]

Use test.skip = '...' instead of in-test logic.

[Peaker]

Add real debugging support, the previous approach was completely broken

[Peaker]

Fix a bug in the way fail objects were passed to Failure in errback()

[therve]

I applied latest patch on the branch, and made some cleanups. That begins to look cool :).

[jknight]

/me wonders upon the purpose of writing it directly in C rather than pyrex.

[therve]

The main purpose of Pyrex is to wrap an existing library. For an optimization, I don't think that's the best choice. Furthermore, memory/GC management with Pyrex can be problematic.

[jknight]

Replying to therve:

The main purpose of Pyrex is to wrap an existing library. For an optimization, I don't think that's the best choice.

If the C implementation is sufficiently faster, that could be a good reason. But it seems to me, speed being equal, the more python-like pyrex should be preferred where possible. Or rpython

Furthermore, memory/GC management with Pyrex can be problematic.

Compared to *C*?

[therve]

Replying to jknight:

If the C implementation is sufficiently faster, that could be a good reason. But it seems to me, speed being equal, the more python-like pyrex should be preferred where possible. Or rpython

Well, yeah, of course, but I really think that the C version is faster than the Pyrex one, which tends to add a lot of useless generated code. We could try the benchmark on both version.

Furthermore, memory/GC management with Pyrex can be problematic.

Compared to *C*?

Pyrex has (had ?) some problems without object deallocation for example. At least with C everything is explicit.

[spiv]

Replying to therve:

Well, yeah, of course, but I really think that the C version is faster than the Pyrex one, which tends to add a lot of useless generated code. We could try the benchmark on both version.

Without actually measuring, statements about how fast they are are worthless. Guessing the actual measurable difference in this sort of situation is notoriously inaccurate.

So benchmark first, then you can argue about whether the various tradeoffs are worth it.

FWIW, unless the difference in performance is stellar, I'd lean towards whichever has the more readable code, which presumably means pyrex. But I want to see a benchmark...

[Peaker]

I initially worked on the Pyrex one from foom.

I spent many many hours trying to debug a GC-related crash that occured with pyrex deferred objects and circular references, when therve asked me if I had tried his c deferred implementation.

I am not sure at all which one is faster, but here is the comparison as I see it:

Pyrex pro's:

1. Pyrex code is more readable and much easier to maintain
2. Pyrex code provides more useful traceback information in its exceptions (builds traceback objects)

C pro's:

1. C code is easier to build
2. C code has more "transparent" memory management/refcounting semantics. With Pyrex, I am always puzzled about how refcounts are managed (e.g: If I cast a <void*> to an <object> do I get a new reference? A burrowed one?)

While I do believe that ease of building is less important, and that the first Pyrex point (easier maintenance) is highly important (it took hours to add the equivalent of a few Python/Pyrex lines to the C one), I just couldn't get the thing to work correctly, memory-management wise.

C memory management is not easy at all, but I believe my patch does all of the refcounting things correctly. I could get it to work with all the circular cases.

I think the comparison of Pyrex/C performance here is probably not an issue. They are both far far faster than the Python defer, and probably similarly so.

If someone can comment on the viability of RPython in terms of ease of build and the other aspects I mentioned, I would be grateful. I just don't know enough about the use of RPython so I haven't tried using it.

[jknight]

Well, I'm sure you noticed the comment in the code where I call PyGC_UnTrack(self) explicitly in dealloc because pyrex doesn't do that automatically. So, yeah, pyrex isn't perfect: you still do need to know the underlying C memory semantics where pyrex's abstraction falls down.

But, I was able to run the twisted unit test suite with my pyrex version w/o crashes. Were you able to do that, but still ran into problems in your own code? Or did that crash for you too?

For ease of building: the generated C code should be distributed along with the .pyx.

For rpython: I don't really know anything about it either, just threw it out there to confuse things further.

[Peaker]

Replying to jknight:

Well, I'm sure you noticed the comment in the code where I call PyGC_UnTrack(self) explicitly in dealloc because pyrex doesn't do that automatically. So, yeah, pyrex isn't perfect: you still do need to know the underlying C memory semantics where pyrex's abstraction falls down. But, I was able to run the twisted unit test suite with my pyrex version w/o crashes. Were you able to do that, but still ran into problems in your own code? Or did that crash for you too? For ease of building: the generated C code should be distributed along with the .pyx. For rpython: I don't really know anything about it either, just threw it out there to confuse things further.

Yes, I have noticed that GC_Untrack call and tried using it.

The bug I had was with an adjusted test of defer's tests. That test may not have existed back when the Pyrex one was created, because it tried assigning into a deferred object's "_d" attribute which did not exist. I converted it to simply assign into "result" which is allowed, and under undeterministic circumstances (When the test is run after some others, but not alone) it caused the GC_Untrack call to crash as the GC header in the object, iirc, was corrupt.

I am not remembering the details exactly, but I do know that I spent less time getting the C one to work, than I did trying to get the Pyrex one to work.

Defer seems relatively stable, so I think that what works correctly *now*, and has all the features, should matter more.

Also, therve wrote cdefer.c with good style, which makes the code reasonably maintainable, even if it is C.

[jknight]

The test in question indeed did not exist: test_defer.py:LogTestCase.test_errorLogWithInnerFrameCycle.

It'd be nice if someone could come up with another way to write that test that doesn't require writing random attributes or writing to .result.

(I had considered making the pyrex deferred not expose any of the attributes but result, and making result be readonly. Might still be a good idea..)

[glyph]

Replying to jknight:

(I had considered making the pyrex deferred not expose any of the attributes but result, and making result be readonly. Might still be a good idea..)

I strongly disagree with the idea of making the C Deferred in any way semantically distinct from the Python Deferred. Even the "private" attributes, which nobody should ever access, are very occasionally helpful for debugging, and the last thing that I want is for the C Deferred to be even harder to debug.

However, I do think agree Deferred has way too many public attributes, which I doubt very much code uses. I would be happy to see these start going away in a gradual and compatibility-friendly way, but I think that should remain separate from this issue.

(I am not totally inflexible on the idea that the new C deferred will not support stuff that is deprecated. However, I am adamant that the deprecations must be done in Python first before we even start talking about dropping features in the C version.)

[jml]

• It is unclear from this ticket whether the cdefer-2245 branch is up-to-date and authoritative.
• Without reading the discussion in-depth, it is difficult to determine whether all of the design and policy questions have been resolved.
• Several patches and comments have been uploaded since the ticket was marked 'review'.

As such, I believe this ticket has not reached the code review stage.

[exarkun]

Peaker is the author of this code.

[Peaker]

You are right, it was indeed not clear.

The cdefer-2245 branch is the up-to-date branch of this ticket.

The modifications that were made after it was set for review were fixes for problems that were discovered after setting for review, but it has been stable since.

[jml]

Are there benchmarks? Have glyph's objections been resolved?

[Peaker]

Replying to jml:

Are there benchmarks? Have glyph's objections been resolved?

Yes. I am not sure what glyph's objection was.

If it was about allowing access to the deferred's "callbacks" attribute, then its not available in the C version. Should it be made available? It should probably be private and thus unavailable. How do you deprecate access to an attribute? I am not sure it makes a lot of sense to do this in the C deferred.

I think if people still depend on silly things like accessing "callbacks" the right way to address that is some kind of way to tell Twisted to use the Python deferred in broken code.

Currently that way is editing defer.py :-) But maybe some better way should be used?

[Peaker]

Report of differences between defer with/without cdefer:

• The callbacks attribute refers to a list of a different structure. In the Py one its [ ((callback, cbargs, cbkw), (errback, ebargs, ebkw))* ]. In the C one its [ (callback, cbargs, cbkw, errback, ebargs, wbkw)* ].
• There is no 'debug' class attribute in cdefer.Deferred. Note that the proper way of getting/setting debug is via getDebugging/setDebugging which are implemented in cdefer.
• Deferred is an old-style class. cdefer.Deferred is a new-style class. This also affects DeferredList or anyone else who inherits from it.
• timeout was finally removed in cdefer, with it the 'timeoutCall' attribute was removed.
• The private _debugInfo attribute is not accessible
• 'callbacks', 'paused', 'called' are read-only attributes in cdeferred but writable in Deferred.

I believe this sums up all of the differences. Which of these are acceptable, and which should be corrected?

Replying to Peaker:

Replying to jml:

Are there benchmarks? Have glyph's objections been resolved?

Yes. I am not sure what glyph's objection was. If it was about allowing access to the deferred's "callbacks" attribute, then its not available in the C version. Should it be made available? It should probably be private and thus unavailable. How do you deprecate access to an attribute? I am not sure it makes a lot of sense to do this in the C deferred. I think if people still depend on silly things like accessing "callbacks" the right way to address that is some kind of way to tell Twisted to use the Python deferred in broken code. Currently that way is editing defer.py :-) But maybe some better way should be used?

[Peaker]

Another difference I missed:

• Class attributes are used for defaults in the .py one, and don't exist in the type object.

Replying to Peaker:

Report of differences between defer with/without cdefer: * The callbacks attribute refers to a list of a different structure. In the Py one its [ ((callback, cbargs, cbkw), (errback, ebargs, ebkw))* ]. In the C one its [ (callback, cbargs, cbkw, errback, ebargs, wbkw)* ]. * There is no 'debug' class attribute in cdefer.Deferred. Note that the proper way of getting/setting debug is via getDebugging/setDebugging which are implemented in cdefer. * Deferred is an old-style class. cdefer.Deferred is a new-style class. This also affects DeferredList or anyone else who inherits from it. * timeout was finally removed in cdefer, with it the 'timeoutCall' attribute was removed. * The private _debugInfo attribute is not accessible * 'callbacks', 'paused', 'called' are read-only attributes in cdeferred but writable in Deferred. I believe this sums up all of the differences. Which of these are acceptable, and which should be corrected? Replying to Peaker:

Replying to jml:

Are there benchmarks? Have glyph's objections been resolved?

Yes. I am not sure what glyph's objection was. If it was about allowing access to the deferred's "callbacks" attribute, then its not available in the C version. Should it be made available? It should probably be private and thus unavailable. How do you deprecate access to an attribute? I am not sure it makes a lot of sense to do this in the C deferred. I think if people still depend on silly things like accessing "callbacks" the right way to address that is some kind of way to tell Twisted to use the Python deferred in broken code. Currently that way is editing defer.py :-) But maybe some better way should be used?

[exarkun]

Okay here are some opinions from me:

• The different callbacks structure is unfortunate. If there is a good reason for this, maybe it's okay. If it's just an accident, then it should probably be made consistent with Deferred.
• It would be best if there were deprecated support for setting cdefer.Deferred.debug. defer.Deferred.debug should also actually be deprecated too. The actual deprecation part of this can be a separate task, though. I don't know how you implement cdefer.Deferred.debug, though.
• I guess it's technically possible to have a classic extension type? I'm not really sure though. I don't think I care about this change though. Maybe we should make it more explicit that subclassing Deferred isn't useful.
• Timeout stuff has been deprecated since 2.0. We should also drop it from defer.py.
• _debugInfo can be considered an implementation detail of defer.py, cdefer doesn't need to provide it.
• I'm a little fuzzy on whether it's okay for those attributes to be read-only. I feel like it is, but maybe that's just a subjective thing, and actually we should follow the deprecation policy for this change.

Sorry for not providing absolutely concrete responses. Maybe if I think about it a while longer I'll be more certain what the correct choices are.

[Peaker]

I deprecated the paused/callbacks attributes now. I also fixed the callbacks list to share the same structure as the Py Deferred list.

Here's an updated list (Now ordered from most substantial to least, IMO):

• 'callbacks', 'paused', 'called' are read-only attributes in cdeferred but writable in Deferred (Now callbacks/paused are deprecated too).
• There is no 'debug' class attribute in cdefer.Deferred. Note that the proper way of getting/setting debug is via getDebugging/setDebugging which are implemented in cdefer.
• Deferred is an old-style class. cdefer.Deferred is a new-style class. This also affects DeferredList or anyone else who inherits from it.
• Class attributes are used for defaults in the .py one, and don't exist in the type object.

I think its reasonable to assume there's a concensus that these are ok:

• timeout was finally removed in cdefer, with it the 'timeoutCall' attribute was removed.
• The private _debugInfo attribute is not accessible

Replying to Peaker:

Another difference I missed: * Class attributes are used for defaults in the .py one, and don't exist in the type object. Replying to Peaker:

Report of differences between defer with/without cdefer: * The callbacks attribute refers to a list of a different structure. In the Py one its [ ((callback, cbargs, cbkw), (errback, ebargs, ebkw))* ]. In the C one its [ (callback, cbargs, cbkw, errback, ebargs, wbkw)* ]. * There is no 'debug' class attribute in cdefer.Deferred. Note that the proper way of getting/setting debug is via getDebugging/setDebugging which are implemented in cdefer. * Deferred is an old-style class. cdefer.Deferred is a new-style class. This also affects DeferredList or anyone else who inherits from it. * timeout was finally removed in cdefer, with it the 'timeoutCall' attribute was removed. * The private _debugInfo attribute is not accessible * 'callbacks', 'paused', 'called' are read-only attributes in cdeferred but writable in Deferred. I believe this sums up all of the differences. Which of these are acceptable, and which should be corrected? Replying to Peaker:

Replying to jml:

Are there benchmarks? Have glyph's objections been resolved?

Yes. I am not sure what glyph's objection was. If it was about allowing access to the deferred's "callbacks" attribute, then its not available in the C version. Should it be made available? It should probably be private and thus unavailable. How do you deprecate access to an attribute? I am not sure it makes a lot of sense to do this in the C deferred. I think if people still depend on silly things like accessing "callbacks" the right way to address that is some kind of way to tell Twisted to use the Python deferred in broken code. Currently that way is editing defer.py :-) But maybe some better way should be used?

[Peaker]

Replying to exarkun:

Okay here are some opinions from me: * The different callbacks structure is unfortunate. If there is a good reason for this, maybe it's okay. If it's just an accident, then it should probably be made consistent with Deferred.

I fixed it to be consistent. There's a minor difference (None instead of passthru) but user code really shouldn't be affected. (1. They should not have looked at defer.callbacks in the first place, 2. They should not compare with passthru, really).

* It would be best if there were deprecated support for setting cdefer.Deferred.debug. defer.Deferred.debug should also actually be deprecated too. The actual deprecation part of this can be a separate task, though. I don't know how you implement cdefer.Deferred.debug, though.

This is difficult, because it requires using a metaclass. I'm attempting to see how horrible it really is though.

* I guess it's technically possible to have a classic extension type? I'm not really sure though. I don't think I care about this change though. Maybe we should make it more explicit that subclassing Deferred isn't useful.

I don't think that the differences between old-style and new-style classes should matter to any reasonable code. Subclassing still works, by the way, it will just behave a bit differently (and subclasses are those that are more likely to notice any differences).

* Timeout stuff has been deprecated since 2.0. We should also drop it from defer.py.

Okay. That's a matter for another ticket, though.

* _debugInfo can be considered an implementation detail of defer.py, cdefer doesn't need to provide it.

That's settled then :-)

* I'm a little fuzzy on whether it's okay for those attributes to be read-only. I feel like it is, but maybe that's just a subjective thing, and actually we should follow the deprecation policy for this change.

Its not hard to allow them to be writable. I thought it simply doesn't "make sense". I guess the correct thing to do is to allow it with a deprecation warning, for now.

Sorry for not providing absolutely concrete responses. Maybe if I think about it a while longer I'll be more certain what the correct choices are.

Thanks for the reply!

[Peaker]

• Added the "debug" attribute with deprecation as alternative to getDebugging/setDebugging.
• Added write access to deprecated "paused" attribute with deprecation warnings.

The differences that still remain:

• called and callbacks are read-only. I can allow modifying them but that's not tested in the current tests and I don't think that much code would be broken by this.
• Deferred is an old-style class. cdefer.Deferred is a new-style class. This also affects DeferredList? or anyone else who inherits from it.
• Class attributes are used for defaults in the .py one, and don't exist in the type object.

In my opinion, the 2nd and 3rd differences are non-issues. The first one is a matter of arbitrary choice - which I made in this way. If you disagree, please explain why.

[therve]

There is warning in the compilation which is suspect to me:

twisted/internet/cdefer/cdefer.c: In function 'cdefer_Deferred_get_callbacks':
twisted/internet/cdefer/cdefer.c:848: warning: implicit declaration of function 'PyErr_WarnEx'
twisted/internet/cdefer/cdefer.c: At top level:
twisted/internet/cdefer/cdefer.c:953: warning: initialization from incompatible pointer type

It seems it has been added in 2.5, we need to support 2.3 and 2.4. The consequence is a failing test.

The changes to twisted.mail.alias must be reverted.

The test methods should be named 'test_foo', not 'testFoo'. They also need docstrings.

If the access to attributes is done in cdefer, it must be done in defer too. That may be done in another ticket, I'm not sure yet.

I'm not sure having specific cases for cdefer is sane. The goal is to have the same behavior (this is related to previous remark).

The benchmark tool have an unused import functools. It would probably need some docstrings too.

That's it for now!

[Peaker]

I toyed with the idea of using properties to deprecate things in the Python Deferred too, before realizing its probably best to just have the C one behave like the old Python one and allow writing on the exposed private attributes of the past. So I am attaching a utility module for twisted.python to allow creation of properties that deprecate attribute access. Due to limitations in "property" in Python, it is probably more useful on new-style classes, if its ever used.

[Peaker]

Thanks for the review. I addressed your comments:

Replying to therve:

There is warning in the compilation which is suspect to me: {{{ twisted/internet/cdefer/cdefer.c: In function 'cdefer_Deferred_get_callbacks': twisted/internet/cdefer/cdefer.c:848: warning: implicit declaration of function 'PyErr_WarnEx' twisted/internet/cdefer/cdefer.c: At top level: twisted/internet/cdefer/cdefer.c:953: warning: initialization from incompatible pointer type }}} It seems it has been added in 2.5, we need to support 2.3 and 2.4. The consequence is a failing test.

I removed the deprecations and the call to PyErr_WarnEx with them. There was also another silly warning I fixed. It compiles without warnings now.

The changes to twisted.mail.alias must be reverted.

Done. Though now they fail because cdefer has no setTimeout. More on that below.

The test methods should be named 'test_foo', not 'testFoo'. They also need docstrings.

Fixed all of test_defer names to be test_foo. Also added docstrings to all of the tests I added, and to some tests that were already there and helper methods. Adding docstrings to _all_ of the defer tests is probably out of the scope of this ticket though.

If the access to attributes is done in cdefer, it must be done in defer too. That may be done in another ticket, I'm not sure yet.

Ok, I changed it to allow access to attributes in c defer in the same way, with writability and no deprecations. Another ticket can be about the privatizing of too many publicly-named attributes of deferred.

I'm not sure having specific cases for cdefer is sane. The goal is to have the same behavior (this is related to previous remark).

Removed those, now that attributes behave the same.

The benchmark tool have an unused import functools. It would probably need some docstrings too.

Fixed.

That's it for now!

One problem remains, as mentioned above, that of setTimeout.

setTimeout must finally be removed for tests to pass with this ticket, because it is not and will not be implemented in the C deferred.

If its done in another ticket, it forms a dependency on it, and this dependency should be mentioned here.

What do you think?

[therve]

• test_reentrantReadCallbacks is failing with PyDeferred
• I'm not sure the change in defer.py about DebugInfo instantiation is relevant here. Can you explain the change?
• Please don't make changes like replacing testMethod by test_method. This is what we want for new methods, but big changes like that just breaks every other branch around modifying test_defer.
• the docstrings must be written

  """
  docstring
  """
  

  even if it's on one line
• there are trailing whitespaces in test_defer

Thanks!

[Peaker]

Thanks for the review! Comments in-place:

Replying to therve:

* test_reentrantReadCallbacks is failing with PyDeferred

Yeah, that's a bug in defer.py (You were not using the c defer one). I fixed it in #2849 and waiting for that to merge before submitting this for review again.

* I'm not sure the change in defer.py about DebugInfo instantiation is relevant here. Can you explain the change?

The change is made to make it easier for the C deferred to use the DebugInfo object. Adding the string prefix from Python is trivial and from C a bit more difficult...

* Please don't make changes like replacing testMethod by test_method. This is what we want for new methods, but big changes like that just breaks every other branch around modifying test_defer.

Ok, I reverted the name changes to existing test methods.

* the docstrings must be written {{{ """ docstring """ }}} even if it's on one line

Fixed.

* there are trailing whitespaces in test_defer

Fixed.

[peaker]

(In [21544]) Initial merge-forward, not ready yet. refs #2245

[peaker]

(In [21547]) Passes the new defer tests, with the new #2849 behavior. refs #2245

[peaker]

(In [21554]) Refactor test_reentrantReadCallbacks. refs #2245

[peaker]

(In [21555]) Remove redundant test (was added in #2849). Fix comment. refs #2245

[Peaker]

I think its basically ready for review, but cannot be merged because twisted.mail.alias still uses setTimeout (arrg!). #2729 is about fixing it, but I am not sure why all that fuss is necessary, as the attached http://twistedmatrix.com/trac/attachment/ticket/2245/cdefer.3.patch fixes it as well (and that specific fix can be put into this branch).

Please give it a look anyhow.

[exarkun]

• doc/core/benchmarks/defer.py
  • missing:
    • copyright notice
    • module docstring (okay it's not really a module, but there should be some overall description of intent)
  • docstrings should be """\ntext\n""", even if they fit on one line (a docstring is a very important thing, it merits the visibility this style provides it)
  • the naming convention is camelCase, not under_scores
  • Isn't pause_unpause basically the same as instantiate_add_callbacks_before_result? Deferred.pause and Deferred.unpause don't get used very frequently.
  • It might be interesting to see Deferred.callback benchmarked on different sized callbacks lists separately from Deferred.addCallbacks. instantiate_add_callbacks_no_result already does the reverse.
• doc/core/benchmarks/timer.py
  • changes seem more or less irrelevant
  • camelCase, not under_scores.
• twisted/test/test_defer.py
  • Thanks for the additional docstrings
  • _verify_cb should probably be _verifyCallbackEntry or something.
  • in docstrings, use I{} or B{} for emphasis (epytext markup), C{} for code literals, L{} for strings which identify a python object (eg, L{Deferred.callbacks}).
  • some trailing whitespace in test_reentrantReadCallbacks
  • test_addCallbackErrbackWithoutArgs could be using assertRaises
• twisted/internet/cdefer.c
  • cdefer_Deferred__runCallbacks has this line: newArgs = Py_BuildValue("(O)", self->result);. A few lines below, if PySequence_InPlaceConcat fails, the function returns without cleaning up newArgs. That's a leak, right?
• bugs I found

  • >>> from twisted.internet.defer import Deferred
    >>> d = Deferred()
    >>> d.callbacks = [None]
    >>> d.callback(None)
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    SystemError: error return without exception set
    >>>
    

  • >>> from twisted.internet.defer import Deferred
    >>> d = Deferred()
    >>> d.callbacks = [0]
    >>> d.callback(None)
    Segmentation fault
    

I should really spend more time looking at the C, but that's a bunch of stuff to start with.

[Peaker]

Replying to exarkun:

* doc/core/benchmarks/defer.py * missing: * copyright notice

Done

* module docstring (okay it's not really a module, but there should be some overall description of intent)

Done

* docstrings should be """\ntext\n""", even if they fit on one line (a docstring is a very important thing, it merits the visibility this style provides it)

Done

* the naming convention is camelCase, not under_scores

Done

* Isn't pause_unpause basically the same as instantiate_add_callbacks_before_result? Deferred.pause and Deferred.unpause don't get used very frequently.

Don't know, the benchmark isn't trying to assume much about the implementation. It is another API so its tested (I remember trying to cover as many of the APIs as reasonable). Benchmark is showing slightly differing results, too.

* It might be interesting to see Deferred.callback benchmarked on different sized callbacks lists separately from Deferred.addCallbacks. instantiate_add_callbacks_no_result already does the reverse.

I guess you could just subtract the time of instantiateAddCallbacksNoResult from the time of instantiateAddCallbacksBeforeResult...

* doc/core/benchmarks/timer.py * changes seem more or less irrelevant

The reason I changed timer.py is to remove the getattr(time, 'time') call from the time measurement. It may be insignificant, but if the iteration count is small, it can have a small effect.

* camelCase, not under_scores.

Done.

* twisted/test/test_defer.py * Thanks for the additional docstrings

No problemos, amigos.

* _verify_cb should probably be _verifyCallbackEntry or something.

Done.

* in docstrings, use I{} or B{} for emphasis (epytext markup), C{} for code literals, L{} for strings which identify a python object (eg, L{Deferred.callbacks}).

I tried fixing that but I am not sure I did it correctly, as I am not sure when to use C{} and when to use L{}. Why is "Deferred" not wrapped in C{} or L{} in all of the docstrings, for example?

* some trailing whitespace in test_reentrantReadCallbacks

Done.

* test_addCallbackErrbackWithoutArgs could be using assertRaises

It is using assertRaises but comes with a comment:

# assertRaises gives a **{} to the function, whereas no arg at all
# gives a NULL to cdefer, so lets call it directly here:

that explains why it is being duplicated in there.

* twisted/internet/cdefer.c * cdefer_Deferred__runCallbacks has this line: newArgs = Py_BuildValue("(O)", self->result);. A few lines below, if PySequence_InPlaceConcat fails, the function returns without cleaning up newArgs. That's a leak, right?

Yeah :-( That code could probably be broken into smaller more verifiable functions. But for now I'll fix that specifically.

* bugs I found ....

Fixed. These bugs result from allowing the user to set 'callbacks', but perhaps it will be better to make 'callbacks', 'paused' and other attributes read only in the context of this ticket? I doubt anyone is depending on the ability to set these attributes on Deferred objects.

I should really spend more time looking at the C, but that's a bunch of stuff to start with.

That's the heavy part of this ticket review...

[Peaker]

The current branch, for anyone wondering, is cdefer-2245-2

[Peaker]

Okay, I just imitate the behavior of pydeferred w.r.t handling of invalid callbacks lists. There's still some difference (some callbacks format failures will just jump to the errback chain in pydeferred but will raise through callback() or errback() in cdefer) but I believe that assigning callbacks is a corner case in itself, and the exact exception behaviour of such assignment that is even _wrong_ is not very important.

Mostly, the behavior is the same, anyhow, and there are no more segfaults. I also added test cases that include both of those previously crashing snippets.

[therve]

Just for info, the branch needs to be merged forward.

[peaker]

(In [21972]) Branching to 'cdefer-2245-3'

[peaker]

(In [21974]) Branching to 'cdefer-2245-3'

[Peaker]

Replying to peaker:

(In [21974]) Branching to 'cdefer-2245-3'

[edsuom]

I tested cdefer-2245-3 with my  AsynCluster installation running on six CPU core nodes. The computing job involves a thousand or so interations with 1000 tasks per iteration, distributed over the six nodes on an as-available basis with  AsynQueue dispatching the tasks from a queue. Four of the nodes, the AsynCluster master process, and the python for the computing job itself ran on an Intel Core QX9650 overclocked to 3.6 GHz. The other two nodes ran on an AMD64 4200+. Most of the time for each of the 1000 tasks per iteration was spent in highly optimized Numpy calls and inline C for array processing.

The first trial was with cdefer-2245-3 as-is. For each module in the dependency code and the computing job with any significant deferred usage, I did a separate import of cdefer in a try:except loop, and then did defer.Deferred = cdefer.Deferred.

For the second trial, I disabled the cdefer capability by renaming the cdefer.so file and cdefer directory in /usr/lib/python2.4/site-packages/twisted/internet.

Here are the iteration times for each, with about a thousand iterations for each trial:

C Deferreds

Min.    1st Quartile  Median  Mean    3rd Quartile    Max. 
0.150   0.630         0.840   0.767   0.950           1.400

Python Deferreds

Min.    1st Quartile  Median  Mean    3rd Quartile    Max. 
0.1900  0.7300        0.8800  0.8033  0.9300          1.4000 

It is probably a tiny bit better with C, but the improvement is marginal indeed for this real-world application. Too bad; I like the idea of optimizing this critical part of Twisted.

A possible next step will be to take dash's suggestion to work on this some more with a profiler.

[ncryptor]

posted on behalf of peaker

Replying to edsuom:

I tested cdefer-2245-3 with my  AsynCluster installation running on six CPU core nodes. The computing job involves a thousand or so interations with 1000 tasks per iteration, distributed over the six nodes on an as-available basis with  AsynQueue dispatching the tasks from a queue. Four of the nodes, the AsynCluster master process, and the python for the computing job itself ran on an Intel Core QX9650 overclocked to 3.6 GHz. The other two nodes ran on an AMD64 4200+. Most of the time for each of the 1000 tasks per iteration was spent in highly optimized Numpy calls and inline C for array processing. The first trial was with cdefer-2245-3 as-is. For each module in the dependency code and the computing job with any significant deferred usage, I did a separate import of cdefer in a try:except loop, and then did defer.Deferred = cdefer.Deferred. For the second trial, I disabled the cdefer capability by renaming the cdefer.so file and cdefer directory in /usr/lib/python2.4/site-packages/twisted/internet. Here are the iteration times for each, with about a thousand iterations for each trial: == C Deferreds == {{{ Min. 1st Quartile Median Mean 3rd Quartile Max. 0.150 0.630 0.840 0.767 0.950 1.400 }}} == Python Deferreds == {{{ Min. 1st Quartile Median Mean 3rd Quartile Max. 0.1900 0.7300 0.8800 0.8033 0.9300 1.4000 }}} It is probably a tiny bit better with C, but the improvement is marginal indeed for this real-world application. Too bad; I like the idea of optimizing this critical part of Twisted. A possible next step will be to take dash's suggestion to work on this some more with a profiler.

I think this is not the main target for cdefer's optimization.

An app that already spends a lot of CPU time doing stuff and a little of its time doing Deferred stuff, will of course gain less than an app that spends almost all of its CPU time dispatching asynchronous calls and calling deferreds.

About your performance measurements: Are these real-time measurements, or CPU time measurements? In this context it probably makes more sense to measure CPU time.

Also, I think the min time probably represents the actual CPU performance best, as non-min times probably contain app-specific and other blocks/delays [including context switches or the specific disk's seek times, etc].

I believe the application-wide min time speed increase of 26% is definitely significant. If Deferreds runtime is no more than X% of your runtime, no matter how fast it will get, you cannot speed up your app by more than a factor of 100/(100-X).

[therve]

(In [23213]) Branching to 'cdefer-2245-4'

[redbaron]

Minimal test-case shows huge memory leaks

[redbaron]

I've attached minimal example of huge memory leaks using latest cdeferred implementation.

[redbaron]

I've no C expirience at all but after reading docs and some experiments commenting out Py_INCREF(self->result); on line 728 seems to solve at least previously attached testcase.

[redbaron]

Even simplier test which shows memory leaks

[redbaron]

I've discovered that memory leaks occurs on every cb or eb. Argument should be relatively big to make memleak noticible, I use Failure() instance for that purpose, I wrap it into my class to show that ordinary callbacks are affected to.

And yes, commenting Py_INCREF(self->result); on line 728 in cdefer.c definetely solves the problem.

[thijs]

Replying to exarkun:

This kind of investigation may be useful, but we need a performance benchmark suite for Deferreds before it can be productive, I think. James Knight has previously implemented Deferred in Pyrex. The reason this isn't included in Twisted is that we just have no reliable, accurate way to measure what the performance changes of this are. The same applies to the attached cdefer.c.

I moved the defer.py from the branch for this ticket to the attachment of #3704 for creating a deferred benchmark. Perhaps we should first complete that benchmark ticket and then move on with this ticket.

[thijs]

Replying to therve:

It could even be a great buildbot metric of Twisted (for problem like #1524).

Running the benchmarks with buildbot is described in #2201

[spiv]

See also #3245, which has a benchmark (for memory consumption).

[vultaire]

I'm quite curious about this branch; just pulled a copy locally. I think for the app we're working on here, this may have a significant impact.

The server I've been working on does massive async calls, and on our last benchmark it appeared to spend over 25% of CPU time in the defer module. About 11% was in inlineCallbacks and friends, and 12% was in the Deferred class itself. (Counting method: sum of immediate CPU time for all functiosn in t.i.defer.py, using cProfile. Note: we use MySQL with adbapi, so I think the threadpool threads could be skewing the results slightly.)

Seems there's supposedly a memory leak mentioned by redbaron, and other than that, this is still just pending review or something? What's the status?

[glyph]

Replying to vultaire:

Seems there's supposedly a memory leak mentioned by redbaron, and other than that, this is still just pending review or something? What's the status?

I could be wrong about this, but unless someone more informed feels like chiming in:

• There's the memory leak you mentioned; that needs to be investigated and fixed.
• I don't believe that the code in this branch implements cancel(), as recently added by #990. That would need to be fixed.
• I think we might be waiting on #411, especially so that the maximum-recursion-depth tests can be applied to this branch.

Merging can be a pain, so if you want to bring this up to date with trunk without svn commit access you probably want to just rebase the whole thing and make a new branch off of a recent trunk. If you'd like to work on these issues, though, please feel free to attach patches or 'bzr send' output.

[vultaire]

Not sure how much time I can commit, but I'd like to see this working.

I can comment on the memory leak issue above. As redbaron mentioned, there was a PyINCREF(self->result) on line 728 in cdefer.c. self->result was assigned from PyObject_Call, which looks like it does its own PyINCREF ( http://docs.python.org/c-api/object.html#PyObject_Call). Removing that PyINCREF looks like it fixes the problem.

The other issues, I can't strongly say I can commit the time right now, but I'll see what I can do.

[exarkun]

Also, this is a big pile of C. It's really unclear that it's going to be worth maintaining, even if you do all the work to fix the points Glyph mentioned. If you can contribute a benchmark that reflects how your application uses Deferreds and demonstrates that this change is actually significantly helpful, that would go a long ways towards proving there's a reason we should undertake this maintenance burden.

[vultaire]

That's fair. I'll see what I can do.

[glyph]

Replying to exarkun:

Also, this is a big pile of C. It's really unclear that it's going to be worth maintaining, even if you do all the work to fix the points Glyph mentioned. If you can contribute a benchmark that reflects how your application uses Deferreds and demonstrates that this change is actually significantly helpful, that would go a long ways towards proving there's a reason we should undertake this maintenance burden.

I would have mentioned benchmarks first, but I thought that #3704 already took care of that? Not that I'd discourage vultaire from adding more benchmarks, of course :).

[exarkun]

Thanks for mentioning that ticket. Any new benchmarks should be written in a way that play nicely with the ones that exist already, and clearly there's no point in overlapping with what's already covered.

Since #3704 was resolved 15 months ago and this ticket is still open, the main conclusion I can draw is that the benchmarks it added don't show any compelling reason to bother with a C implementation of Deferred. That may simply be because they benchmarks there aren't realistic (but it may also be because C Deferred doesn't speed up real programs).

[therve]

The C extension does speed up real programs, I've observed it in a few scenarios. Does it make a big difference? The most I've seen is about 20%, so I'm not such if it really worths the trouble. Maintaining C is hard. But the test coverage of !Deferred is not as bad is it used to be, especially with the tests in #411.

I guess this code needs a champion for the time being, but I'd rather try to tackle #411 if I find some time.

[therve]

Gah, I'm not sure it's really worth the trouble.

[vultaire]

Well, if it's not, we could end up just using it locally for our purposes. That'd be fine; I can understand that in most cases this is likely not the bottleneck for a program.

That being said, I see a difference in a benchmark I wrote between the two, and I'll check out the benchmarks of #3704 as well. I'll agree that I would expect more from a C extension.

But I'm thinking there's a few places the extension could still be improved upon where more performance could be won. If I can get something compelling together, I'll share it.

[vultaire]

Bleh, rather vague last comment. So far I've seen a 20+% difference between cdefer and defer in overall time in one benchmark I've done so far. Not as high of a boost as I would expect for a C extension.

And of course, on a real system that's doing real work like database queries, caching, running some algorithms on the data, etc., the above 20% boost may translate to only 7% or less on overall time. Probably not worth the effort as-is. Hopefully it's possible to make this a bit faster...

[mkeller@…]

I am working for a project that heavily uses Deferreds (viff.dk). Using an adapted version of the C extension, our benchmarks run 50 to 100 percent faster. Therefore, I would like to use the code, but there is no copyright notice in the patches. Is the license the same as for the whole project? Who do I have to credit?