classification
Title: PEP 3121, 384 Refactoring applied to decimal module
Type: resource usage Stage: resolved
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: later
Dependencies: Superseder:
Assigned To: skrah Nosy List: Robin.Schreiber, ezio.melotti, loewis, mark.dickinson, mrabarnett, pitrou, rhettinger, skrah
Priority: normal Keywords: pep3121

Created on 2012-08-18 12:31 by Robin.Schreiber, last changed 2014-10-14 17:07 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
_decimal_pep3121-384_v0.patch Robin.Schreiber, 2012-08-18 12:31
_decimal_pep3121-384_v1.patch Robin.Schreiber, 2012-08-18 22:36
Messages (12)
msg168511 - (view) Author: Robin Schreiber (Robin.Schreiber) * Date: 2012-08-18 12:31
Changes proposed in PEP3121 and PEP384 have now been applied to the decimal module!
msg168513 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-18 14:05
Thank you for the patch and the big amount of work that you are doing on so
many modules!

I'm afraid though that the patch is not acceptable in its current state:

    1) The unit tests do not pass. This could be fixed.

    2) The patch slows down _decimal by 25% (!).


If 2) cannot be fixed due to the increased amount of state lookups, I'm
firmly -1 on applying pep-3121 changes to _decimal (and any other speed
sensitive module).
msg168526 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-18 20:00
> 2) The patch slows down _decimal by 25% (!).

Ouch. That's an interesting observation, because I guess other modules could be affected as well.

Robin, can you please take care that other modules don't have significant performance regressions after your work? I'm not sure what kind of changes it involves, or if it's possible at all, but we don't want to slow down Python for what is mostly (IMO) a code cleanup.
msg168536 - (view) Author: Robin Schreiber (Robin.Schreiber) * Date: 2012-08-18 22:36
I have removed some redundant modulestate lookups and the testsuite now executes the decimal tests as fast as before the patch is applied. (at least for me).
May I ask how you tested the decimal performance?

Regarding the failing test:
It appears that the hackcheck() method in typeobject.c is responsible for this failure: 

static int
hackcheck(PyObject *self, setattrofunc func, char *what)
{
    PyTypeObject *type = Py_TYPE(self);
    while (type && type->tp_flags & Py_TPFLAGS_HEAPTYPE)
        type = type->tp_base;
    /* If type is NULL now, this is a really weird type.
       In the spirit of backwards compatibility (?), just shut up. */
    if (type && type->tp_setattro != func) {
        PyErr_Format(PyExc_TypeError,
                     "can't apply this %s to %s object",
                     what,
                     type->tp_name);
        return 0;
    }
    return 1;
}

As the context-type is now a heaptype the while-loop will continue to jump to the basetype of the conext-type, which is the object-type. There it ultimately fails when it compares func (context_setattr) to the tp_setattr of object.
Anyway, I do not understand the purpose of the hackcheck method, so I can not propose any kind of solution for this problem.
msg168550 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-19 07:45
The test suite is not a good benchmark: it also tests decimal.py. For
numerical performance I'm running:

  cd Modules/_decimal/tests
  ../../../python bench.py

You can hit Ctrl-C after the first cdecimal result, since that's usually
already a pretty good indicator of overall performance. On 64-bit, for
9 digits of precision cdecimal is currently only around 1.5 times slower
than float. I want to keep that.

Running an unpatched _decimal.c three times gives (Linux, 64-bit, Core2 Duo):

0.162576s  0.165146s  0.163242s

With your second patch:

0.204383s  0.204383s  0.206919s

> Regarding the failing test:
> It appears that the hackcheck() method in typeobject.c is responsible for this failure: 

Thanks for the analysis. Perhaps Martin can comment on that.
msg168551 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-08-19 08:20
hackcheck fixes the "Carlo Verry hack", which goes like this:

py> object.__setattr__(str, 'lower', str.upper)
py> 'dammit Carlo!'.lower()
'DAMMIT CARLO!'
(from http://bugs.jython.org/issue1058)

It shouldn't be possible to monkey-patch a builtin type; I believe this is to prevent crashes when self has the incorrect layout. Other than that, I find that an arbitrary restriction, except that setattr/attribute assignment prevent an assignment from occurring, it shouldn't be possible to bypass this by calling __setattr__. So if the restriction could be lifted, it should be lifted in both cases.

What specific decimal test is failing?
msg168552 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-08-19 09:40
> What specific decimal test is failing?

# Attributes cannot be deleted
    for attr in ['prec', 'Emax', 'Emin', 'rounding', 'capitals', 'clamp',
                 'flags', 'traps']:
        self.assertRaises(AttributeError, c.__delattr__, attr)

test test_decimal failed -- Traceback (most recent call last):
  File "/home/stefan/pydev/pep-3121-cpython/Lib/test/test_decimal.py", line 3683, in test_invalid_context
    self.assertRaises(AttributeError, c.__delattr__, attr)
  File "/home/stefan/pydev/pep-3121-cpython/Lib/unittest/case.py", line 571, in assertRaises
    return context.handle('assertRaises', callableObj, args, kwargs)
  File "/home/stefan/pydev/pep-3121-cpython/Lib/unittest/case.py", line 135, in handle
    callable_obj(*args, **kwargs)
TypeError: can't apply this __delattr__ to object object

1 test failed:
    test_decimal
msg222098 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-07-02 12:48
In order to avoid the significant slowdown:  Could we create a new
kind of method (METH_STATE) and change ceval to pass a state struct
that contains the thread and the module state as the first parameter
if the METH_STATE flag is present?
msg222200 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-07-03 16:52
This sounds like a question for python-dev (or perhaps python-ideas).
msg222213 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-07-03 20:47
Yes, python-ideas is probably better. -- I just noticed that the
total slowdown is even 40% if the global variable "cached_context"
is also placed into the module state (as it should).
msg222738 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-07-11 11:51
Sorry Robin, I was wrong about the context -- it should be fine
since it's thread-local.  So the slowdown is back to 25%.
msg229317 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-10-14 17:07
I would like to reject this until either the performance problems
are solved or someone actually uses _decimal in multiple interpreters.

If you do, you get tons of warnings about libmpdec being reinitialized,
so I suspect no one has ever done that in Python 3.3+ (I can't imagine
that such scary warnings would not be reported).
History
Date User Action Args
2014-10-14 17:07:05skrahsetstatus: open -> closed
resolution: later
messages: + msg229317

stage: resolved
2014-07-11 11:51:04skrahsetmessages: + msg222738
2014-07-03 20:47:52skrahsetmessages: + msg222213
2014-07-03 16:52:51ezio.melottisetmessages: + msg222200
2014-07-02 12:48:41skrahsetmessages: + msg222098
2012-11-08 13:36:48Robin.Schreibersetkeywords: + pep3121, - patch
2012-08-27 03:49:13belopolskylinkissue15787 dependencies
2012-08-19 09:40:13skrahsetmessages: + msg168552
2012-08-19 08:20:39loewissetmessages: + msg168551
2012-08-19 07:45:04skrahsetmessages: + msg168550
2012-08-18 22:36:40Robin.Schreibersetfiles: + _decimal_pep3121-384_v1.patch

messages: + msg168536
2012-08-18 20:00:04pitrousetnosy: + loewis, pitrou
messages: + msg168526
2012-08-18 14:08:41skrahsetassignee: skrah
2012-08-18 14:05:29skrahsetnosy: + rhettinger, mark.dickinson
messages: + msg168513
2012-08-18 13:11:01Robin.Schreibersetcomponents: + Extension Modules, - Regular Expressions
2012-08-18 12:31:10Robin.Schreibercreate