Skip to content

PEP 3121, 384 Refactoring applied to decimal module #59927

Closed
Closed
PEP 3121, 384 Refactoring applied to decimal module#59927
Assignees
skrah
Labels
extension-modulesC modules in the Modules dirperformancePerformance or resource usage

Description

@RobinSchreiber
RobinSchreiber
mannequin
opened on Aug 18, 2012
BPO 15722
Nosy @loewis, @rhettinger, @mdickinson, @pitrou, @ezio-melotti, @skrah
Files
  • _decimal_pep3121-384_v0.patch
  • _decimal_pep3121-384_v1.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/skrah'
closed_at = <Date 2014-10-14.17:07:05.421>
created_at = <Date 2012-08-18.12:31:10.125>
labels = ['extension-modules', 'performance']
title = 'PEP 3121, 384 Refactoring applied to decimal module'
updated_at = <Date 2014-10-14.17:07:05.420>
user = 'https://bugs.python.org/RobinSchreiber'

    bugs.python.org fields:

    activity = <Date 2014-10-14.17:07:05.420>
actor = 'skrah'
assignee = 'skrah'
closed = True
closed_date = <Date 2014-10-14.17:07:05.421>
closer = 'skrah'
components = ['Extension Modules']
creation = <Date 2012-08-18.12:31:10.125>
creator = 'Robin.Schreiber'
dependencies = []
files = ['26888', '26889']
hgrepos = []
issue_num = 15722
keywords = ['pep3121']
message_count = 12.0
messages = ['168511', '168513', '168526', '168536', '168550', '168551', '168552', '222098', '222200', '222213', '222738', '229317']
nosy_count = 8.0
nosy_names = ['loewis', 'rhettinger', 'mark.dickinson', 'pitrou', 'ezio.melotti', 'mrabarnett', 'skrah', 'Robin.Schreiber']
pr_nums = []
priority = 'normal'
resolution = 'later'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue15722'
versions = ['Python 3.4']

    Activity

    1. RobinSchreiber commented on Aug 18, 2012

      @RobinSchreiber
      RobinSchreibermannequin
      on Aug 18, 2012
      MannequinAuthor

      Changes proposed in PEP-3121 and PEP-384 have now been applied to the decimal module!

    2. RobinSchreiber
      added
      topic-regex
      performancePerformance or resource usage
      extension-modulesC modules in the Modules dir
      and removed
      topic-regex
      on Aug 18, 2012

    3. skrah commented on Aug 18, 2012

      @skrah
      skrahmannequin
      on Aug 18, 2012
      Mannequin

      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).

    4. skrah
      self-assigned this
      on Aug 18, 2012

    5. pitrou commented on Aug 18, 2012

      @pitrou
      pitrou
      on Aug 18, 2012
      Member
      1. 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.

    6. RobinSchreiber commented on Aug 18, 2012

      @RobinSchreiber
      RobinSchreibermannequin
      on Aug 18, 2012
      MannequinAuthor

      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.

    7. skrah commented on Aug 19, 2012

      @skrah
      skrahmannequin
      on Aug 19, 2012
      Mannequin

      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.

    8. loewis commented on Aug 19, 2012

      @loewis
      loewismannequin
      on Aug 19, 2012
      Mannequin

      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?

    9. skrah commented on Aug 19, 2012

      @skrah
      skrahmannequin
      on Aug 19, 2012
      Mannequin

      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

    10. skrah commented on Jul 2, 2014

      @skrah
      skrahmannequin
      on Jul 2, 2014
      Mannequin

      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?

    11. ezio-melotti commented on Jul 3, 2014

      @ezio-melotti
      ezio-melotti
      on Jul 3, 2014
      Member

      This sounds like a question for python-dev (or perhaps python-ideas).

    12. skrah commented on Jul 3, 2014

      @skrah
      skrahmannequin
      on Jul 3, 2014
      Mannequin

      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).

    13. skrah commented on Jul 11, 2014

      @skrah
      skrahmannequin
      on Jul 11, 2014
      Mannequin

      Sorry Robin, I was wrong about the context -- it should be fine
      since it's thread-local. So the slowdown is back to 25%.

    14. skrah commented on Oct 14, 2014

      @skrah
      skrahmannequin
      on Oct 14, 2014
      Mannequin

      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).

    15. skrah
      closed this as completedon Oct 14, 2014
    16. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    extension-modulesC modules in the Modules dirperformancePerformance or resource usage

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions