Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mode to disable small integer and interned string caches #79048

Closed
stevendaprano opened this issue Oct 2, 2018 · 11 comments
Closed

Add mode to disable small integer and interned string caches #79048

stevendaprano opened this issue Oct 2, 2018 · 11 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@stevendaprano
Copy link
Member

BPO 34867
Nosy @nascheme, @rhettinger, @terryjreedy, @gpshead, @jwilk, @stevendaprano, @njsmith, @serhiy-storchaka, @ammaraskar

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 = None
closed_at = None
created_at = <Date 2018-10-02.00:40:12.328>
labels = ['interpreter-core', 'type-feature', '3.8']
title = 'Add mode to disable small integer and interned string caches'
updated_at = <Date 2018-11-30.20:00:54.989>
user = 'https://github.com/stevendaprano'

bugs.python.org fields:

activity = <Date 2018-11-30.20:00:54.989>
actor = 'terry.reedy'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2018-10-02.00:40:12.328>
creator = 'steven.daprano'
dependencies = []
files = []
hgrepos = []
issue_num = 34867
keywords = []
message_count = 10.0
messages = ['326838', '326843', '326850', '326852', '326853', '326854', '326860', '327073', '327093', '330823']
nosy_count = 9.0
nosy_names = ['nascheme', 'rhettinger', 'terry.reedy', 'gregory.p.smith', 'jwilk', 'steven.daprano', 'njs', 'serhiy.storchaka', 'ammar2']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue34867'
versions = ['Python 3.8']

@stevendaprano
Copy link
Member Author

Split off from bpo-34850 by Guido's request.

To help catch incorrect use of is when == is intended, perhaps we should add an interpreter mode that disables the caches for small ints and interned strings.

Nathaniel called it "chaos mode" but I don't like the name as there is nothing chaotic about the lack of such caches, and it doesn't come close to chaos testing (e.g. Netflix's Chaos Monkey tool).

@stevendaprano stevendaprano added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Oct 2, 2018
@ammaraskar
Copy link
Member

Maybe something more akin to UndefinedBehaviorSanitizer? Since its supposed to be catching implementation specific quirks. It wouldn't really be sanitizing though, more just making the bugs more likely to appear.

@serhiy-storchaka
Copy link
Member

Adding a runtime option will hit a performance of normal execution. And it is impossible to disable interning strings completely. Some core code depends on this. I have also concerns about disabling caching an empty string.

There are also other caches on different levels.

@ammaraskar
Copy link
Member

Serhiy, take a look at the linked ticket. The idea is that something like pytest or libregrtest will use this to bring underlying bugs to the surface. It isn't intended to be used in normal execution.

@serhiy-storchaka
Copy link
Member

I don't worry about the performance when caches are disabled. An additional check will hit the performance in normal execution.

@ammaraskar
Copy link
Member

Aah sorry, I misinterpreted what you meant. The original ticket proposes it as a compile time flag as well.

@rhettinger
Copy link
Contributor

I don't think this option will be of any value. For it to work, the code would need to have this particular bug, have test cases that triggered those bugs, and a user sophisticated enough to run the tests but unsophisticated enough to make beginner mistakes regarding when to use identity tests versus equality tests (something I teach on day one of beginner Python courses).

Before this goes further, I would like to see some evidence that it would actually catch a real bug in the wild.

@nascheme
Copy link
Member

nascheme commented Oct 4, 2018

Woudn't turning these off hurt performance a lot? If so, I don't know if people would actually use such a mode. Then it becomes pretty useless. Could we combine this idea with the PYTHONDEVMODE flag? If PYTHONDEVMODE is turned on, we could do a check like Serhiy suggests for inappropriate 'is' comparisons. That seems more useful to me.

@gpshead
Copy link
Member

gpshead commented Oct 4, 2018

The intent is to use only enable this during testing / continuous integration.

@terryjreedy
Copy link
Member

Steven, thank you for splitting this off for proper discussion.

To me, the base issue is that CPython is both the language reference implementation and, as yet, the main production implementation. As the latter, it has unintended and unwanted bugs and intentional optimizations added for performance rather than language conformance. Some of these, like caching, affect boolean results involving 'is' and id(). Problems arise when people confuse reference features with implementation features.

This issue proposes adding a mode that turns off certain optimization features. There is another proposal to turn off other optimizations (again during code analysis and testing) that affect tracing results and sometimes coverage results based thereon, giving false negatives. In either case, I see the result as a 'language reference' mode. As Steven suggested, the result is in a sense less chaotic, not more. A chaos mode for caching would randomly cache or not.

Multiple comments above contain 'bug'. Given that the language leaves implementations to cache certain immutables -- or not -- the bug in code meant to be implementation independent is to depend on caching *either way*. Turning caching off only catches the 'bug' of assuming caching, not the bug of assuming no caching.

From a math viewpoint, n is n for all n, so 'is' *is* the proper comparison for ints. From this viewpoint, caching should be the default and having not caching most values of n, and having to use '==' instead of 'is', is the practice time-space tradeoff compromise.

Like Raymond, I currently think that this proposal lacks sufficient justification.

@terryjreedy terryjreedy added the type-feature A feature request or enhancement label Nov 30, 2018
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel
Copy link
Member

Three core devs objected to this proposal. Closing.

@iritkatriel iritkatriel closed this as not planned Won't fix, can't repro, duplicate, stale Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

8 participants