[issue32630] Migrate decimal to use PEP 567 context variables

Gregory P. Smith added the comment: FYI - this appears to have caused a regression - https://bugs.python.org/issue39776 -- nosy: +gregory.p.smith ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Change by Yury Selivanov : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: New changeset f13f12d8daa587b5fcc66fe3ed1090a5dadab289 by Yury Selivanov in branch 'master': bpo-32630: Use contextvars in decimal (GH-5278) https://github.com/python/cpython/commit/f13f12d8daa587b5fcc66fe3ed1090a5dadab289 --

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: Thank you, Stefan. I've updated the PR with an asyncio+decimal test and run tests in refleak mode to make sure there's no regression there. If during the beta/rc period we see that contextvars isn't stable enough or something I'll

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: On Fri, Jan 26, 2018 at 11:11:00PM +, STINNER Victor wrote: > vstinner@apu$ ./python -m perf compare_to master.json pr5278.json > Mean +- std dev: [master] 1.86 us +- 0.03 us -> [pr5278] 2.27 us +- 0.04 us: > 1.22x slower (+22%) > >

[issue32630] Migrate decimal to use PEP 567 context variables

STINNER Victor added the comment: > Likewise, on the same builds, running _decimal/tests/bench.py does not show a > significant difference: > https://gist.github.com/elprans/fb31510ee28a3aa091aee3f42fe65e00 Note: it may be interesting to rewrite this benchmark my

[issue32630] Migrate decimal to use PEP 567 context variables

STINNER Victor added the comment: Since the root of the discussion is a performance regression, let me take a look since I also care of not regressing in term of performance. We (CPython core developers, as as team) spent a lot of time on optimizing CPython to make

[issue32630] Migrate decimal to use PEP 567 context variables

Guido van Rossum added the comment: Apologies accepted. I did not imply that -- I was simply stating that Yury needed your help reproducing your result so he could do something about it. It seems you two are taking this offline so I trust that there will be no more barbs.

[issue32630] Migrate decimal to use PEP 567 context variables

Elvis Pranskevichus added the comment: Likewise, on the same builds, running _decimal/tests/bench.py does not show a significant difference: https://gist.github.com/elprans/fb31510ee28a3aa091aee3f42fe65e00 -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Elvis Pranskevichus added the comment: FWIW, I ran bm_telco with pyperformance on a benchmark-tuned system and did not observe the slowdown. Benchmarks were done on a release build (--enable-optimizations) $ sudo (which python3) -m perf system tune MASTER: $ pyperformance

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: Guido, I apologize for the outburst. I had the impression that msg310799 implicitly asserted my incompetence in benchmarking. -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: Yury, would you be willing to work this out by email? -- I think it was you who I discussed the context-subclassing with and that was quite a pleasant experience. -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Guido van Rossum added the comment: Stefan this is unacceptable abuse. Please read the code of conduct. -- ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: Sorry Stefan, I never wanted this to look like "I'm pushing this without listening to Stefan". I apologize if it looked that way. I ran bm_telco on my machine before submitting the PR, and I indeed did not see any performance impact.

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: I have run about 1000 times more decimal benchmarks than both Yury and you. You attempt to hurt my reputation is laughable. Show me some top-performance code that you have written. -- ___ Python

[issue32630] Migrate decimal to use PEP 567 context variables

Guido van Rossum added the comment: Stefan, I don't think a module author should retain veto over everything affecting their code forever. (We've had spectacular process failures with this in the past.) Please take a deep breath and patiently answer Yury's questions. If you

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: Guido, I have the feeling that the feature -- about which I was actually positive in the first place -- is being pushed aggressively with no respect for the module author. BTW, prec is changed quite frequently in decimal code, so if people

[issue32630] Migrate decimal to use PEP 567 context variables

Guido van Rossum added the comment: Guys. Please stop with the editorializing. "I cannot believe ..." (used essentially by both of you) is not constructive. -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: On Fri, Jan 26, 2018 at 09:06:38PM +, Yury Selivanov wrote: > This benchmark is specially constructed to profile creating decimal contexts > and doing almost nothing with the It is not constructed at all. It was the first thing I wrote

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: > However, I could not find any tests for the added feature (safe > use with async) though. We would be adding a new feature without > tests. This is no problem, I can add a few async/await tests. > I'm getting a large slowdown: >

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: Tests - I ran some of my own tests (not even close to all), they seem fine. However, I could not find any tests for the added feature (safe use with async) though. We would be adding a new feature without tests. Performance ---

[issue32630] Migrate decimal to use PEP 567 context variables

Guido van Rossum added the comment: You guys both need to calm down. Stefan, what's your objection against this, assuming the crash is fixed? -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: > Sure, and *I* am the one running the extended decimal test suite as we speak, > not Guido. Thank you. > You are playing power games here, and you did that from the start by choosing > the nosy list. Please. I thought it was pretty

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: Sure, and *I* am the one running the extended decimal test suite as we speak, not Guido. You are playing power games here, and you did that from the start by choosing the nosy list. -- ___ Python

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: Stefan, I do think that this is a release blocker. We want to get this change as early as possible to ensure that it's well tested. AFAIK Guido also wants decimal to be updated and well supported in async/await code. --

[issue32630] Migrate decimal to use PEP 567 context variables

Change by Stefan Krah : -- priority: release blocker -> normal ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Change by Yury Selivanov : -- nosy: +ned.deily priority: normal -> release blocker ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Change by Mark Dickinson : -- nosy: +mark.dickinson ___ Python tracker ___ ___

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: I pushed a fix (already in the master branch) and rebased the patch once again. I expect it to work now :) -- ___ Python tracker

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: I think I found what cause this, but I have no idea why it has surfaced only now :) https://github.com/python/cpython/pull/5326/files (see the added PyType_IS_GC(Py_TYPE(name)) check) I'll merge that PR and rebase the decimal patch

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: (Just in case I rebased my patch onto the latest master) -- ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: > I realize that you had to fight massive mailing list distractions > during the PEP discussions, but this is very close to the beta ... Oh thanks, but I see no reason for you to be condescending here. I cannot reproduce this on Mac OS /

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: I realize that you had to fight massive mailing list distractions during the PEP discussions, but this is very close to the beta ... Let's start here: >>> from decimal import * ==18887== Invalid read of size 8 ==18887==at 0x5324E0:

[issue32630] Migrate decimal to use PEP 567 context variables

Yury Selivanov added the comment: Stefan, it would be great to have this committed before 3.7 feature freeze. The change is pretty straightforward -- we replaced threading.local() with a contextvar, which should be a backwards compatible change. --

[issue32630] Migrate decimal to use PEP 567 context variables

Stefan Krah added the comment: I'll take a look. -- assignee: -> skrah ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

Change by Yury Selivanov : -- keywords: +patch pull_requests: +5122 ___ Python tracker ___

[issue32630] Migrate decimal to use PEP 567 context variables

New submission from Yury Selivanov : PEP 567 allows decimal to be safely used in async/await code. I couldn't observe any performance impact by the proposed PR. The PR doesn't modify decimal context behaviour: instead of using a thread-local storage it now uses a