Author meador.inge
Recipients benjamin.peterson, bob.ippolito, collinwinter, georg.brandl, giampaolo.rodola, inducer, jafo, mark.dickinson, meador.inge, nnorwitz, piman
Date 2010-03-08.14:11:30
SpamBayes Score 2.77556e-16
Marked as misclassified No
Message-id <1268057494.32.0.481232329062.issue1530559@psf.upfronthosting.co.za>
In-reply-to
Content
> (2) For 2.x, I'm a bit uncomfortable with introducing the extra Python layer 
> on top of the C layer.  Partly I'm worried about accidentally breaking 
> something (it's not 100% clear to me whether there might be hidden 
> side-effects to such a change),

I understand.  I am worried about that as well. The main area that 
concerns me is the interface for the 'Struct' class.  I did my best to 
match the methods and functions from the C implementation.  I need to 
look into this in more detail, though.  One quirk I currently know 
about is that the following methods:

   '__delattr__', '__getattribute__', '__setattr__', '__new__'

show up in 'help' for the C implementation, but not the Python one.  
Although, the implementations do exist in both places. 

> but I also notice that this seems to have a significant impact on performance.
> In fact, I seem to recall that the previously existing Python component of 
> the struct module was absorbed into Modules/_struct.c precisely for 
> performance reasons.
>
> A quick, unscientific benchmark:  the time taken to run test_struct with this
> patch (excluding the changes to test_struct itself) on my machine (OS X 10.6,
> 64-bit non-framework non-debug build of Python) is around 1.52--1.53 seconds; > without the patch it's around 1.02--1.03 seconds.

Agreed that this is not a really scientific benchmark.  I did 
experiment with this idea a little further though on my machine (OS X 
10.5 32-bit):

==============================================
| Configuration                    | Seconds |
==============================================
| Original                         | 1.26    |
----------------------------------------------
| __index__ patch v1               | 1.88    |
----------------------------------------------
| Hoisted imports out of functions | 1.53    |
----------------------------------------------
| Hoisted imports and no __index__ | 1.34    |
----------------------------------------------

So with this simple experiment pulling the 'imports' out of the function
wrappers made quite a difference.  And, of course, removing the 
'__index__' transform brought the times down even more.  So, the 
wrapper overhead does not look to be terrible for this simple test.

However, as you alluded to, we should find a better benchmark.  Any 
ideas on a good one?
 
> (4) For 2.x, perhaps we don't need the extra __index__ functionality anyway, 
> now that the previous use of __int__ has been restored.  That would give us 
> a bit more time to think about this for 3.x.

Agreed.  Thanks for taking the time to look at the patch anyway!
History
Date User Action Args
2010-03-08 14:11:34meador.ingesetrecipients: + meador.inge, nnorwitz, georg.brandl, collinwinter, jafo, bob.ippolito, mark.dickinson, piman, inducer, giampaolo.rodola, benjamin.peterson
2010-03-08 14:11:34meador.ingesetmessageid: <1268057494.32.0.481232329062.issue1530559@psf.upfronthosting.co.za>
2010-03-08 14:11:32meador.ingelinkissue1530559 messages
2010-03-08 14:11:30meador.ingecreate