Deprecate ob_shash in BytesObject

@tiran

BPO	46864
Nosy	@tiran, @methane, @serhiy-storchaka, @animalize, @brandtbucher
PRs	bpo-46864: Deprecate PyBytesObject.ob_shash. #31598 bpo-46864: Suppress deprecation warnings for ob_shash. #32042 bpo-46864: Suppress even more ob_shash deprecation warnings (GH-32176) #32176
Files	remove-bytes-hash.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 = None
closed_at = <Date 2022-03-25.02:15:16.155>
created_at = <Date 2022-02-26.08:49:05.075>
labels = ['interpreter-core', 'build', '3.11']
title = 'Deprecate ob_shash in BytesObject'
updated_at = <Date 2022-03-30.06:35:35.207>
user = 'https://github.com/methane'

bugs.python.org fields:

activity = <Date 2022-03-30.06:35:35.207>
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date = <Date 2022-03-25.02:15:16.155>
closer = 'methane'
components = ['Interpreter Core']
creation = <Date 2022-02-26.08:49:05.075>
creator = 'methane'
dependencies = []
files = ['50649']
hgrepos = []
issue_num = 46864
keywords = ['patch']
message_count = 21.0
messages = ['414083', '414096', '414134', '414136', '414605', '415684', '415743', '415744', '415747', '415748', '415749', '415854', '415855', '415857', '415865', '415923', '415925', '415927', '415958', '415984', '416325']
nosy_count = 5.0
nosy_names = ['christian.heimes', 'methane', 'serhiy.storchaka', 'malin', 'brandtbucher']
pr_nums = ['31598', '32042', '32176']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue46864'
versions = ['Python 3.11']

Code objects have more and more bytes attributes for now.
To reduce the RAM by code, I want to remove ob_shash (cached hash value) from bytes object.

Sets and dicts have own hash cache.
Unless checking same bytes object against dicts/sets many times, this don't cause big performance loss.

I think it is a legacy of Python 2. Attributes and variable names are Unicode strings in Python 3, so the main reason of this optimization is no longer relevant.

But some programs can still work with encoded bytes instead of strings. In particular os.environ and os.environb are implemented as dict of bytes on non-Windows.

But some programs can still work with encoded bytes instead of strings. In particular os.environ and os.environb are implemented as dict of bytes on non-Windows.

This change doesn't affect to os.environ.

os.environ[key] does key.encode(sys.getfilesystemencoding(), "surrogateescape") internally. So the encoded key doesn't have cached hash.
On the other hand, dict (self._data) has own hash cache. So it don't use hash cached in the bytes objects.

On the other hand, this change will affect os.environb[key] if key is used repeatedly.

When removed shash:

## small key
$ ./python -m pyperf timeit --compare-to ../cpython/python -s 'd={b"foo":1, b"bar":2, b"buzz":3}' -- 'b"key" in d'
/home/inada-n/work/python/cpython/python: ..................... 23.2 ns +- 1.7 ns
/home/inada-n/work/python/remove-bytes-hash/python: ..................... 40.0 ns +- 1.5 ns

Mean +- std dev: [/home/inada-n/work/python/cpython/python] 23.2 ns +- 1.7 ns -> [/home/inada-n/work/python/remove-bytes-hash/python] 40.0 ns +- 1.5 ns: 1.73x slower

## large key
$ ./python -m pyperf timeit --compare-to ../cpython/python -s 'd={b"foo":1, b"bar":2, b"buzz":3};k=b"key"*100' -- 'k in d'
/home/inada-n/work/python/cpython/python: ..................... 22.3 ns +- 1.2 ns
/home/inada-n/work/python/remove-bytes-hash/python: ..................... 108 ns +- 2 ns

Mean +- std dev: [/home/inada-n/work/python/cpython/python] 22.3 ns +- 1.2 ns -> [/home/inada-n/work/python/remove-bytes-hash/python] 108 ns +- 2 ns: 4.84x slower

I will reconsider the removal before remove the cache.
We changed code object too often. If Python 3.13 don't use so much bytes objects, we don't need to remove the hash to save some RAM.

New changeset 2d8b764 by Inada Naoki in branch 'main':
bpo-46864: Deprecate PyBytesObject.ob_shash. (GH-31598)
2d8b764

I'm getting tons of deprecation warnings from deepfreeze script. Please add _Py_COMP_DIAG_IGNORE_DEPR_DECL to Tools/scripts/deepfreeze.py.

This has been brought up in NumPy, NumPy subclasses bytes, and I am not immediately sure how to best initialize the hash value.

Would you give me a link to the code?

Would it make sense to move the -1 initialization to a custom tp_alloc? Or am I missing an obvious solution? If this the slow is removed entirely, then it does not matter of course.

How about tp_init?

@methane

@methane the code is here:
https://github.com/numpy/numpy/blob/46a61b3aed3cf674586b456b05c6c9b7b9c5020d/numpy/core/src/multiarray/scalarapi.c#L687

With the initialization a bit later.

The point being, this creates a bytes subclass the manual way currently. I am not condoning that, but I am also not sure there is an obvious replacement for creating a byte subclass instances. FromStringAndSize would be the right thing, but that won't give the subclass.

EDIT: The point about init/new is that they are Python level API, and as such expect args/kwargs. And I don't really think that would be ideal here either. I don't mind this requiring a bit of a hackish solution, but right now I am not sure what the solution is at all.

Thank you. tp_alloc seems good.

What are problems with calling __new__ (tp_new)?

@serhiy-storchaka

@serhiy-storchaka it requires passing a Python object to create the new bytes (subclass) instance. NumPy does not have such an object available. We would have to create it just to call bytes.__new__()... There is a reason the C-API provides PyBytes_FromStringAndSize, but we can't use it for a subclass.

Now, I don't care about using "unstable" API. So if the category of "unstable" is created (and the discussion and Petr's last mail sound a bit like it), then it would make sense to me if this ABI detail was available only through that? Otherwise a custom tp_alloc initializing this to -1 makes perfect sense to me, but maybe there are e.g. micro-optimization reasons against it.

Current ideas:

Create custom .tp_alloc()
Add new PyTypeObject *type parameter to _PyBytes_FromSize() and expose it as a cpython API.

BTW, Since bytes object is PyVarObject, every subclasses won't extend the PyBytesObject, am I right?

BTW, Since bytes object is PyVarObject, every subclasses won't extend the PyBytesObject, am I right?

NumPy certainly does not, I suppose you could dynamically add more "items" at the end in a custom tp_alloc, though? Again, I am happy with changes or removal of ob_shash (so long it is not in a bug-fix release).
It just would be nice to avoid the DeprecationWarning in a clean way

The happy path of the code in question is:

    PyTypeObject *type = /* get the type object */;
    int itemsize = /* get the string 'length' */;
    PyObject *obj;
    void *destptr;
    obj = type->tp_alloc(type, itemsize);
    destptr = PyBytes_AS_STRING(obj);
    ((PyBytesObject *)obj)->ob_shash = -1;
    memcpy(destptr, data, itemsize);
    return obj;

How should NumPy refactor that?

#if PY_VERSION_HEX < 0x030b00b0
    ((PyBytesObject *)obj)->ob_shash = -1;
#endif

gh-91020: Add `PyBytes_Type.tp_alloc` for subclass (GH-91686)

I recommend keeping the ob_shash field. The memory use is negligible — 25% on tiny bytes instances and diminishing further in significance as the sizes get larger.

The caching benefit doesn't happen often but when it does the benefits are significant. Crypto hashes are always stored as bytes and we use them to check for duplicates (i.e. the way git works). The output of str.encode() is bytes and it is legitimate to use this as a cache key (with the lru_cache for example). If I recall correctly, DNA sequence fragments get stored as bytes and are used for repeated dict/set lookups.

This code has been stable for a very long time and it is almost certain that some code in the wild depends on cached hash values for efficient computation. ISTM the deprecation damages that code for no good reason.

Looking back over the thread, it seems that the original motivations are mostly gone or are insignificant. Unless someone really wants this, I request that the field be undeprecated.

Memory saving is not the only reason for removing the hash cache.
Please read my previous comment.

Additionally, this issue doesn't remove the hash cache.
This issue only deprecate direct access to PyBytesObjects.ob_shash.
Even if we won't remove the hash cache, direct access to struct member is not recommended.

The hash cache should remain. If you want to block direct access to the structure member that is reasonable.

For code that does use the hash cache, it is performance critical. The feature has been in a place a long time. Code in the wild almost certainly relies on it (and reasonably so) for good performance. Saying "that CPUs are fast" is dismissive and ignore the essential fact that bytes can be of any size. At some size (likely somewhat small), a fast CPU computation loses to a field lookup, especially for a field that is likely already loaded in the cache line. In contrast, the data array is much less likely to be in the current cache line, so its memory accesses will be expensive.

I've ready you "reasons" and still object to removing the hash cache. Please undo the deprecation.

Navigation Menu

Repository navigation

Deprecate ob_shash in BytesObject #91020

20 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Deprecate ob_shash in BytesObject #91020

Description

Activity

methane commented on Feb 26, 2022

serhiy-storchaka commented on Feb 26, 2022

methane commented on Feb 27, 2022

methane commented on Feb 27, 2022

methane commented on Mar 6, 2022

tiran commented on Mar 21, 2022

20 remaining items

methane commented on Apr 12, 2022

seberg commented on Apr 12, 2022

methane commented on Apr 12, 2022

serhiy-storchaka commented on Apr 12, 2022

seberg commented on Apr 12, 2022

methane commented on Apr 13, 2022

seberg commented on Apr 13, 2022

mattip commented on Apr 19, 2022

methane commented on Apr 19, 2022

rhettinger commented on Nov 4, 2022

methane commented on Nov 4, 2022

rhettinger commented on Nov 5, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions