You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Will Pierce (JIRA)" <ji...@apache.org> on 2011/03/09 14:30:59 UTC
[jira] Created: (THRIFT-1088) Thrift library python classes should
be new-style classes
Thrift library python classes should be new-style classes
---------------------------------------------------------
Key: THRIFT-1088
URL: https://issues.apache.org/jira/browse/THRIFT-1088
Project: Thrift
Issue Type: Improvement
Components: Python - Library
Affects Versions: 0.6
Reporter: Will Pierce
Priority: Minor
The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
The following thrift classes don't inherit from the 'object' class, so are old-style classes:
{noformat}
% find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
./src/protocol/TCompactProtocol.py:class CompactType:
./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
./src/protocol/TProtocol.py:class TProtocolBase:
./src/protocol/TProtocol.py:class TProtocolFactory:
./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
./src/Thrift.py:class TType:
./src/Thrift.py:class TMessageType:
./src/Thrift.py:class TProcessor:
./src/server/TServer.py:class TServer:
./src/server/TNonblockingServer.py:class Connection:
./src/server/TNonblockingServer.py:class TNonblockingServer:
./src/transport/TTransport.py:class TTransportBase:
./src/transport/TTransport.py:class CReadableTransport:
./src/transport/TTransport.py:class TServerTransportBase:
./src/transport/TTransport.py:class TTransportFactoryBase:
./src/transport/TTransport.py:class TBufferedTransportFactory:
./src/transport/TTransport.py:class TFramedTransportFactory:
{noformat}
I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Jake Farrell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004579#comment-13004579 ]
Jake Farrell commented on THRIFT-1088:
--------------------------------------
+1 looks good
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004810#comment-13004810 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
Oh, I follow you. No problem.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Attachment: THRIFT-1088_python_new_style_classes_v2.patch
updated patch to work against trunk because THRIFT-1093 changed adjacent lines in lib/py/src/protocol/TCompactProtocol.py
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, THRIFT-1088_python_new_style_classes_v2.patch, perftest.py, runtest.sh, runtest2.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004630#comment-13004630 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
This patch works fine in my own application code.
I just learned how to run the test suite in the trunk/, which is a win, but a bigger win is that all tests passed from running 'make check' in the trunk/test/ directory. (see output below)
This was my first ever attempt to run the thrift tests, so I'd appreciate if someone more experienced than me could confirm that the changes pass all tests.
My test output (grepped):
{noformat}
% make check 2>&1 | egrep '^All|Ran|OK|PASS|FAIL|==='
Ran 8 tests in 0.002s
OK
PASS: SerializationTest.py
Ran 4 tests in 0.002s
OK
PASS: TestEof.py
PASS: TestSyntax.py
Ran 20 tests in 0.018s
OK
Ran 20 tests in 0.022s
OK
Ran 20 tests in 0.021s
OK
Ran 20 tests in 0.042s
OK
Ran 20 tests in 0.027s
OK
Ran 20 tests in 0.043s
OK
PASS: RunClientServer.py
==================
All 4 tests passed
==================
{noformat}
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Attachment: perftest.py
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Attachment: THRIFT-1088_pthon_new_style_classes.patch
svn diff attached, I am currently testing this change.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004693#comment-13004693 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
Sure thing. Without fastbinary.so in play?
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004660#comment-13004660 ]
David Reiss commented on THRIFT-1088:
-------------------------------------
Can you run a benchmark on this. The reason that generated classes are not new-style by default is that it actually did produce a substantial performance regressions (I think about 1.5x) in [de]serialization.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004751#comment-13004751 ]
David Reiss commented on THRIFT-1088:
-------------------------------------
No, I meant the generated classes. When the new_style option was originally added, the first patch submitted just made all of the generated classes new-style. We changed it to an off-by-default option because of the performance hit.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13005061#comment-13005061 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
Here's the test results from my first stab at some performance testing. I'll attach two more files, perftest.py and runtest.sh that generates the following output:
h2. Test of unpatched source
Starting test, python=sys.version_info(major=2, minor=7, micro=0, releaselevel='final', serial=0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 119.4833302 usec/iteration | 11.948333 total time
| test_oldgen | 118.4334683 usec/iteration | 11.843347 total time
| test_newgen_accel | 22.4877405 usec/iteration | 2.248774 total time
| test_oldgen_accel | 22.5729895 usec/iteration | 2.257299 total time
h3. Running test with python2.4 (no accelerated protocol, not built for this python)
Starting test, python=(2, 4, 6, 'final', 0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 130.6353498 usec/iteration | 13.063535 total time
| test_oldgen | 128.4458494 usec/iteration | 12.844585 total time
| test_newgen_accel | 134.2011499 usec/iteration | 13.420115 total time
| test_oldgen_accel | 133.1284285 usec/iteration | 13.312843 total time
h2. Test of patched code
bq.
patching file lib/py/src/protocol/TBinaryProtocol.py
patching file lib/py/src/protocol/TCompactProtocol.py
patching file lib/py/src/protocol/TProtocol.py
patching file lib/py/src/server/TServer.py
patching file lib/py/src/server/TNonblockingServer.py
patching file lib/py/src/Thrift.py
patching file lib/py/src/transport/TTransport.py
Starting test, python=sys.version_info(major=2, minor=7, micro=0, releaselevel='final', serial=0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 119.9114180 usec/iteration | 11.991142 total time
| test_oldgen | 118.4970808 usec/iteration | 11.849708 total time
| test_newgen_accel | 21.5028381 usec/iteration | 2.150284 total time
| test_oldgen_accel | 21.6538477 usec/iteration | 2.165385 total time
h3. Running test with python2.4 (no accelerated protocol, not built for this python)
Starting test, python=(2, 4, 6, 'final', 0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 131.5409899 usec/iteration | 13.154099 total time
| test_oldgen | 129.8144007 usec/iteration | 12.981440 total time
| test_newgen_accel | 136.0489106 usec/iteration | 13.604891 total time
| test_oldgen_accel | 135.2320910 usec/iteration | 13.523209 total time
Backing out patch (-R for reverse)
patching file lib/py/src/protocol/TBinaryProtocol.py
patching file lib/py/src/protocol/TCompactProtocol.py
patching file lib/py/src/protocol/TProtocol.py
patching file lib/py/src/server/TServer.py
patching file lib/py/src/server/TNonblockingServer.py
patching file lib/py/src/Thrift.py
patching file lib/py/src/transport/TTransport.py
Done
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Patch Info: [Patch Available]
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Attachment: runtest.sh
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13006390#comment-13006390 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
% ./runtest2.sh
Building libraries
make[1]: Leaving directory `/home/willp/src/thrift-svn/trunk'
h2. Test of unpatched source
Starting test, python=sys.version_info(major=2, minor=7, micro=0, releaselevel='final', serial=0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 116.6607690 usec/iteration | 11.666077 total time
| test_oldgen | 114.1114902 usec/iteration | 11.411149 total time
| test_newgen_accel | 22.4592090 usec/iteration | 2.245921 total time
| test_oldgen_accel | 21.6746688 usec/iteration | 2.167467 total time
h3. Running test with python2.4 (no accelerated protocol, not built for this python)
Starting test, python=(2, 4, 6, 'final', 0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 122.2611308 usec/iteration | 12.226113 total time
| test_oldgen | 119.1130900 usec/iteration | 11.911309 total time
| test_newgen_accel | 127.0162797 usec/iteration | 12.701628 total time
| test_oldgen_accel | 123.3603191 usec/iteration | 12.336032 total time
h2. Test of patched code
patching file lib/py/src/protocol/TBinaryProtocol.py
patching file lib/py/src/protocol/TCompactProtocol.py
patching file lib/py/src/protocol/TProtocol.py
patching file lib/py/src/server/TServer.py
patching file lib/py/src/server/TNonblockingServer.py
patching file lib/py/src/Thrift.py
patching file lib/py/src/transport/TTransport.py
Building libraries
make[1]: Leaving directory `/home/willp/src/thrift-svn/trunk'
Starting test, python=sys.version_info(major=2, minor=7, micro=0, releaselevel='final', serial=0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 120.1339102 usec/iteration | 12.013391 total time
| test_oldgen | 118.3011293 usec/iteration | 11.830113 total time
| test_newgen_accel | 21.6843987 usec/iteration | 2.168440 total time
| test_oldgen_accel | 21.5187597 usec/iteration | 2.151876 total time
h3. Running test with python2.4 (no accelerated protocol, not built for this python)
Starting test, python=(2, 4, 6, 'final', 0), iterations=100000, tests: ('test_newgen', 'test_oldgen', 'test_newgen_accel', 'test_oldgen_accel')
| test_newgen | 131.1226416 usec/iteration | 13.112264 total time
| test_oldgen | 129.4400001 usec/iteration | 12.944000 total time
| test_newgen_accel | 136.2670994 usec/iteration | 13.626710 total time
| test_oldgen_accel | 135.5415392 usec/iteration | 13.554154 total time
Backing out patch (-R for reverse)
patching file lib/py/src/protocol/TBinaryProtocol.py
patching file lib/py/src/protocol/TCompactProtocol.py
patching file lib/py/src/protocol/TProtocol.py
patching file lib/py/src/server/TServer.py
patching file lib/py/src/server/TNonblockingServer.py
patching file lib/py/src/Thrift.py
patching file lib/py/src/transport/TTransport.py
Done
... ... ...
My observations, reformated into a table:
|| Python version || Generated-class types || Accelerated Proto? || Unpatched elapsed time/iteration || Patched time/iteration || Performance difference
| 2.7 | New-style | No | 116.6607690 usec | 120.1339102 usec | +2.977%
| 2.7 | Old-style | No | 114.1114902 usec | 118.3011293 usec | +3.672%
| 2.7 | New-style | Yes| 22.4592090 usec | 21.6843987 usec | -3.450%
| 2.7 | Old-style | Yes| 21.6746688 usec | 21.5187597 usec | -0.719%
| 2.4 | New-style | No | 122.2611308 usec | 120.1339102 usec | -1.740%
| 2.4 | Old-style | No | 119.1130900 usec | 129.4400001 usec | +8.670%
The accelerated protocol (fastbinary.so) in the python2.7 testing shows little change in speed. The speedup of 3.5% is real and even grows to ~4.5% on longer runs. I don't know why, but I would guess python 2.7 has really improved new-style class invocation from C extension code. The python 2.7 pure-python (non-accel) numbers show a 2.9 - 3.6% slowdown with the patch. The performance difference is smallest when using new-style generated classes, for some reason.
Going back to older python 2.4.6, which I don't have the fastbinary.so built for, so no accelerated protocol numbers, there is a small ~2% speedup for new-style generated code, and a ~8% slowdown for old-style generated code. I ran more tests of py2.4 with more iterations (50,000) and got a 120.3 usec vs. 128.15 usec difference, or a +6.52% slowdown on the old-style generated code. Since I don't have fastbinary.so built against python2.4, I don't know if there's much of a speed difference for that case, but I think it's probably about the same as the 2.7 testing- marginal differences.
The test code itself isn't the most representative of python use cases, since it only serializes, so if anyone want to enhance the perftest.py code to be more representative, that would help. (I tried to add TCompactProtocol to the test case, but actually uncovered a bug in the encoder, so I'll submit a fix for that in a different ticket.)
I hope this is useful information.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh, runtest2.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13005070#comment-13005070 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
To set up the perf test code, put perftest.py and runtest.sh into trunk/ and put the patch file THRIFT-1088_python_new_style_classes.patch in the directory above trunk. Make sure the thrift compiler is in your path, and run ./runtest.sh, which will run perftest.py with the installed python and with python2.4, then it applies the patch, and re-runs the same tests, then backs out the patch (with patch -R).
This is just a first stab. I couldn't actually get much of a performance difference between old/new in the generated code, so I might not be stressing the differences enough.
Take a look, let me know what you think...
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004696#comment-13004696 ]
David Reiss commented on THRIFT-1088:
-------------------------------------
I guess with and without. Hopefully it won't make any difference with.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (THRIFT-1088) Thrift library python classes should
be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Will Pierce updated THRIFT-1088:
--------------------------------
Attachment: runtest2.sh
Fixed runtest.sh (now called runtest2.sh) shell script so it will re-run 'make' before initial perftest.py run, and after patching the source but before it runs perftest.py.
The previous runtest.sh didn't actually test the patch, because the perftest.py code includes the files from ./lib/py/build/lib* not ./lib/py/src/ So this updated runtest2.sh script ensures the ./lib/py/build/lib* directories are updated before each run.
I'll post new performance measurements output in a minute.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch, perftest.py, runtest.sh, runtest2.sh
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (THRIFT-1088) Thrift library python classes
should be new-style classes
Posted by "Will Pierce (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1088?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13004702#comment-13004702 ]
Will Pierce commented on THRIFT-1088:
-------------------------------------
Quick question, when you say 'generated classes', do you mean the non-generated classes?
The compiler gives the option to generate old-style or new-style classes at the users' preference. New style generated classes still only are made when the user asks explicitly for them with: thrift -gen py:new_style filename.thrift
I definitely will do a benchmark with new-style thrift library classes and each of generated old-style and new-style classes for the generated code, since it's probably interesting anyhow.
> Thrift library python classes should be new-style classes
> ---------------------------------------------------------
>
> Key: THRIFT-1088
> URL: https://issues.apache.org/jira/browse/THRIFT-1088
> Project: Thrift
> Issue Type: Improvement
> Components: Python - Library
> Affects Versions: 0.6
> Reporter: Will Pierce
> Priority: Minor
> Attachments: THRIFT-1088_pthon_new_style_classes.patch
>
>
> The included python thrift library classes (TType, TProtocolBase, etc...) are all defined using old-style python class definitions. This makes it somewhat more difficult to dynamically deal with instances of those classes at runtime, since the type() builtin doesn't return the class name. I don't think there's really any good reason for avoiding using new style classes. The typical fear/uncertainty/doubt with new-style classes (since 2001 when they were made available in python 2.2) is that they are thought to be slower than old-style classes. I personally haven't ever found the performance difference between old and new style classes to
> Unless there's a compelling reason that Thrift developers want to use old-style classes, I think it's a more programmer-friendly choice to switch these to new-style classes. Not to mention the benefits that new style classes provide, see: http://realmike.org/blog/2010/07/18/introduction-to-new-style-classes-in-python/
> The following thrift classes don't inherit from the 'object' class, so are old-style classes:
> {noformat}
> % find . -name \*py | xargs grep class | grep -v [.]svn | grep '[a-zA-Z0-9]:$'
> ./src/protocol/TCompactProtocol.py:class CompactType:
> ./src/protocol/TCompactProtocol.py:class TCompactProtocolFactory:
> ./src/protocol/TProtocol.py:class TProtocolBase:
> ./src/protocol/TProtocol.py:class TProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolFactory:
> ./src/protocol/TBinaryProtocol.py:class TBinaryProtocolAcceleratedFactory:
> ./src/Thrift.py:class TType:
> ./src/Thrift.py:class TMessageType:
> ./src/Thrift.py:class TProcessor:
> ./src/server/TServer.py:class TServer:
> ./src/server/TNonblockingServer.py:class Connection:
> ./src/server/TNonblockingServer.py:class TNonblockingServer:
> ./src/transport/TTransport.py:class TTransportBase:
> ./src/transport/TTransport.py:class CReadableTransport:
> ./src/transport/TTransport.py:class TServerTransportBase:
> ./src/transport/TTransport.py:class TTransportFactoryBase:
> ./src/transport/TTransport.py:class TBufferedTransportFactory:
> ./src/transport/TTransport.py:class TFramedTransportFactory:
> {noformat}
> I'll make changes to my local copy of trunk, run the tests and then attach a patch to this ticket for evaluation.
> Unless anyone has objections or concerns about switching to new style classes?
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira