[Twisted-Python] Ticket #8244 (old-style decorator)

[L. Daniel Burr]

Hi all,

I tried to comment on the ticket, but SpamBayes rejected it as spam.

As a person who runs twisted apps via Pypy whenever possible, I wanted to point out that this ticket may result in a performance regression: according to http://pypy.org/performance.html, "Classes that inherit from both new- and old-style classes are extremely slow; avoid at all costs."

Thanks,

Daniel
--
L. Daniel Burr
***@me.com
[phone]

[Glyph]

Post by L. Daniel Burr
Hi all,

Hi Daniel,

Post by L. Daniel Burr
I tried to comment on the ticket, but SpamBayes rejected it as spam.

The spam-monitoring queue is empty, which means another admin probably got to this before I did. But when I plugged the following paragraph in, something about it made spambayes think it was still 70% likely that it was spam. So, I re-trained the filter repeatedly until it came out to <1%; you should have less trouble with it in the future.

Post by L. Daniel Burr
As a person who runs twisted apps via Pypy whenever possible, I wanted to point out that this ticket may result in a performance regression: according to http://pypy.org/performance.html <http://pypy.org/performance.html>, "Classes that inherit from both new- and old-style classes are extremely slow; avoid at all costs."

In fact, the opposite is true! Right now, every new Twisted class must be new-style, so if it inherits from an old-style class we end up in this situation. Since most core Twisted superclasses are old-style, that means this happens all the time.

Nothing about 8244 would involve making more hybrid classes. Classes decorated as @oldStyle must be pure old style (the semantics of hybrids are much, much closer to new-style than old-style) so they have to be what they are today. When we flip the switch there will be no more old-style classes at all.

Is there a scenario I'm missing / not understanding about the way the ticket's steps are outlined?

-glyph

[L. Daniel Burr]

Hi Glyph,

On March 22, 2016 at 11:27:10 PM, Glyph (***@twistedmatrix.com) wrote:


On Mar 22, 2016, at 12:33 PM, L. Daniel Burr <***@me.com> wrote:

Hi all,

Hi Daniel,

I tried to comment on the ticket, but SpamBayes rejected it as spam.

The spam-monitoring queue is empty, which means another admin probably got to this before I did.  But when I plugged the following paragraph in, something about it made spambayes think it was still 70% likely that it was spam.  So, I re-trained the filter repeatedly until it came out to <1%; you should have less trouble with it in the future.


Thanks for looking into that, I appreciate it.

As a person who runs twisted apps via Pypy whenever possible, I wanted to point out that this ticket may result in a performance regression: according to http://pypy.org/performance.html, "Classes that inherit from both new- and old-style classes are extremely slow; avoid at all costs."

In fact, the opposite is true!  Right now, every new Twisted class must be new-style, so if it inherits from an old-style class we end up in this situation.  Since most core Twisted superclasses are old-style, that means this happens all the time.

Nothing about 8244 would involve making more hybrid classes.  Classes decorated as @oldStyle must be pure old style (the semantics of hybrids are much, much closer to new-style than old-style) so they have to be what they are today.  When we flip the switch there will be no more old-style classes at all.

Is there a scenario I'm missing / not understanding about the way the ticket's steps are outlined?


I’m thinking of a twisted.web service at the moment.  Right now, most of the classes involved (resource.Resource, http.HTTPChannel, http.Request and server.Request, etc.) are old-style classes, and pypy does a reasonable job of optimizing those, if not to the degree that new-style classes may be optimized.  My concern is that, should these classes be converted into hybrids via the @oldStyle decorator, my service may experience some reduction in the number of requests per second that it handles.  Obviously I’m using pypy because it gives my twisted.web service a substantial improvement in terms of the number of requests per second that it can handle.

I understand that I can set TWISTED_NEWSTYLE to 0 and sidestep this potential performance hit, but it is a detail that has to be communicated to various devops teams, has to be kept track of across environments, should probably be removed once a fully-converted release of Twisted arrives, and so on.

Yes, I’m being a bit of a whiner here (apologies), and clearly the feature can be disabled, and I should definitely test the results to see if there *is* a performance regression in my case.

Thanks,

Daniel
--
L. Daniel Burr
***@me.com
[phone]

[Glyph]

If oldStyle converts the classes into hybrids, then it's broken. Hybrids don't follow the semantics of old-style classes. So this won't happen.

Post by L. Daniel Burr
I should definitely test the results to see if there *is* a performance regression in my case.

My guess is that this is an insignificant micro-optimization and that pypy has gotten better at optimizing hybrids since that caution was written. That said, we're really bad at paying attention to speed.twistedmatrix.com <http://speed.twistedmatrix.com/> and we really need a way to see pre-merge performance metrics rather than just a graph over time.

-g

[L. Daniel Burr]

Hi Glyph,
On March 23, 2016 at 1:39:53 PM, Glyph (***@twistedmatrix.com) wrote:


On Mar 23, 2016, at 9:22 AM, L. Daniel Burr <***@me.com> wrote:

My concern is that, should these classes be converted into hybrids via the @oldStyle decorator, my service may experience some reduction in the number of requests per second that it handles

If oldStyle converts the classes into hybrids, then it's broken.  Hybrids don't follow the semantics of old-style classes.  So this won't happen.

 I should definitely test the results to see if there *is* a performance regression in my case.

My guess is that this is an insignificant micro-optimization and that pypy has gotten better at optimizing hybrids since that caution was written.  That said, we're really bad at paying attention to speed.twistedmatrix.com and we really need a way to see pre-merge performance metrics rather than just a graph over time.


In the interest of putting my money where my mouth is, I’ve tested a very simple case, where I create a class inheriting from twisted.web.resource.Resource, and a subclass which mixes in “object”:

from twisted.application import internet, service
from twisted.internet import endpoints, reactor
from twisted.web import resource, server


class MyResource(resource.Resource):
isLeaf = True

def render_GET(self, request):
return 'Hello, World!'

class FrankenStyle(MyResource, object):
pass

print type(MyResource)
print type(FrankenStyle)

application = service.Application(‘test')
root = FrankenStyle()
s = server.Site(root)
ep = endpoints.serverFromString(reactor, 'tcp:8080')
svc = internet.StreamServerEndpointService(ep, s)
svc.setName(‘hybridtest')
svc.setServiceParent(application)

Running siege --benchmark --concurrent=10 --reps=1000 "http://127.0.0.1:8080/“ against the “MyResource” version gets me 5714.29 requests per second across 3 runs.
Running siege --benchmark --concurrent=10 --reps=1000 "http://127.0.0.1:8080/“ against the “FrankenStyle” version gets me an average of 5671.61 requests per second across 3 runs.
So, not a huge difference, overall, and it might not even be visible over a long enough period of time.
Thanks,
Daniel

[Glyph]

Post by L. Daniel Burr
Hi Glyph,

Post by Glyph
If oldStyle converts the classes into hybrids, then it's broken. Hybrids don't follow the semantics of old-style classes. So this won't happen.

Post by L. Daniel Burr
I should definitely test the results to see if there *is* a performance regression in my case.

My guess is that this is an insignificant micro-optimization and that pypy has gotten better at optimizing hybrids since that caution was written. That said, we're really bad at paying attention to speed.twistedmatrix.com <http://speed.twistedmatrix.com/> and we really need a way to see pre-merge performance metrics rather than just a graph over time.

Thanks for doing this.

Post by L. Daniel Burr
So, not a huge difference, overall, and it might not even be visible over a long enough period of time.

Glad to hear it :).

With this in mind, my recommendations to most Twisted users is to start tacking on ", object" to the end[1] of your inheritance hierarchies, so that you get hybrid classes now, which will help you sort out any problems you will have by going fully new-style, at the cost of a small performance hit. It'll be easier to suffer through <1% performance hit (0.7% by your metrics) for the next few releases and change nothing when the new-style release happens, than to be concerned about it breaking stuff.

-glyph

[1]: The end and ONLY THE END of the hierarchy; if you put 'object' first then when those classes go new-style you'll get an exception when declaring the class.

[Tom Prince]

Post by Glyph
Nothing about 8244 would involve making more hybrid classes. Classes
much, much closer to new-style than old-style) so they have to be what they
are today. When we flip the switch there will be no more old-style classes
at all.

Nothing about #8264's *requirements* leads to hybrid classes. But the
implementation makes *every* currently old-style class into a
hybrid-class..

[Glyph]

Post by Tom Prince
Nothing about #8264's *requirements* leads to hybrid classes. But the
implementation makes *every* currently old-style class into a
hybrid-class..

Looks like the implementation needs to be revisited then :).

-glyph

[Amber Brown]

_______________________________________________
Twisted-Python mailing list
Twisted-***@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python