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
_sre.compile(): be more strict on types of indexgroup and groupindex #72951
Comments
Attached patch makes _sre.compile() more strict on types: groupindex must be a dictionary and indexgroup must be a tuple. Currently, indexgroup is passed as a list. I chose to convert it to a tuple to use less memory and to prevent unwanted changes. For unwanted changed, I'm not sure because groupindex remains a mutable dictionary. Do you think that it's worth it to require a tuple? Another option is to accept a list but to convert it to a list, but this change is more specific to the current implementation. |
If make groupindex and indexgroup a dict and a tuple, we could use concrete dict and tuple API instead of generic mapping and sequence APIs. Note that groups, groupindex and indexgroup are not independent. Actually indexgroup is derived from groups and groupindex (in Python code), but groups and groupindex could be derived from indexgroup. groups could be derived from code. The interface of _sre.compile() can be simplified by passing only groupindex or indexgroup. Current interface is only for backward compatibility. I don't know whether this still is needed. |
New changeset 1addc5d2c246 by Victor Stinner in branch 'default': |
sre_compile_remove_groups.patch removes the groups parameter from _sre.compile(). A first step to simplify the API. I prefer to keep most of the code in pure Python, to have code easier to maintain. So I prefer to not accept only groupindex. I prefer to build both (indexgroup, groupindex) in pure Python and pass them to the C code. I pushed my patch sre_types.patch. |
Following patch replaces abstract types API with concrete types API and makes the memory consumption of the pattern object smaller if there are no named groups. $ ./python -m perf timeit -s "import re; m = re.match('(?P<first>first) (?P<second>second)', 'first second')" -- "m.lastgroup"
Unpatched: Median +- std dev: 89.8 ns +- 1.8 ns
Patched: Median +- std dev: 80.5 ns +- 3.3 ns
$ ./python -m perf timeit -s "import re; m = re.match('(?P<first>first) (?P<second>second)', 'first second')" -- "m.groupdict()"
Unpatched: Median +- std dev: 803 ns +- 16 ns
Patched: Median +- std dev: 711 ns +- 16 ns
$ ./python -m perf timeit -s "import re; m = re.match('(?P<first>first) (?P<second>second)', 'first second')" -- "m['first']"
Unpatched: Median +- std dev: 228 ns +- 14 ns
Patched: Median +- std dev: 217 ns +- 11 ns |
sre-concrete.patch is wrong: if groupindex is a subtype of dict, you should use PyObject_GetItem. I like the idea of using PyDict_GetItem, you should just implement stricter checks in _sre.compile(). Maybe using PyDict_CheckExact? Since it's a private module, we don't have to support all types. It's ok to implement further optimizations. |
1addc5d2c246 broke support of non-dict mappings, this patch brakes support of dict subclasses with overridden __getitem__. I think we can ignore this as well. |
Sorry, my comment is unclear: I'm only asking your to add PyDict_CheckExact check in _sre.compile() for sre-concrete.patch. |
Should we check that code is exact list? |
Do you mean a tuple for indexgroup? Yeah, it also seems worth it. I just checked: class Tuple(tuple):
def __getitem__(self, index):
return 9
a = Tuple([1, 2, 3])
print(a[0]) Output: |
It is too. But I meant the code parameter. |
Updated patch restricts _sre.compile() to accepting only exact collection types. I don't like this patch. |
Oh ok. It seems like we use PyList_xxx() functions, so yeah, it seems safer (to respect the Python semantics) to only accept exactly the list type.
Why not? Do you prefer to accept subtypes? sre-concrete-2.patch LGTM, I just added a minor comment. |
I don't like it because it adds a cumbersome code for guarding against a case that doesn't happen in practice. This is just unpythonic. _sre.compile() is a private function and it is called only with exact types. Even if it would called with subtypes, most subtypes don't override __len__ and __getitem__. This restriction is too strong. |
Hum, sorry, I don't understand what is the issue with adding a few addition checks in the constructor? Is it a matter of speed?
Wait, what? I asked you to add checks to not break the Python semantics. If you want to be "Pythonic": you must support the Python semantics, so support when __getitem__, __len__, etc. are replaced. I don't understand your "unpythonic" argument.
Right. So what is the problem with adding more sanity checks?
It's a matter of respecting the Python semantics. |
An optimization must not change the behaviour of Python. |
sre-concrete-2.patch LGTM, so do you want to push it Serhiy? |
It is a matter of readability and maintainability. Additional checks adds 21 lines of the code, and they hardcode error messages. I copied currently produced error messages, with hardcoded function name and argument numbers. Changes in PyArg* functions or Argument Clinic that could globally enhance error messages would not affect these ones. Changing the function name or the arguments order would need to change error messages.
Sorry, I used bad wording. I meant that this doesn't follow the practice of CPython code. Builtins and extensions often ignore subclassing and use concrete API for simplicity and speed. Especially in internal functions. The interface of _sre.compile() already is made less general, it no longer works with general sequences and mappings. Ignoring subclassing don't make it much worse. |
The patch makes some operations slightly faster. $ ./python -m perf timeit -s 'import re; m = re.match(r"(?P<int>\d+)(?:\.(?P<frac>\d*))?", "12.34")' -- --duplicate 100 'm.lastgroup'
Unpatched: Median +- std dev: 204 ns +- 1 ns
Patched: Median +- std dev: 167 ns +- 2 ns
$ ./python -m perf timeit -s 'import re; m = re.match(r"(?P<int>\d+)(?:\.(?P<frac>\d*))?", "12.34")' -- --duplicate 100 'm.groupdict()'
Unpatched: Median +- std dev: 2.78 us +- 0.05 us
Patched: Median +- std dev: 1.98 us +- 0.04 us
$ ./python -m perf timeit -s 'import re; m = re.match(r"(?P<int>\d+)(?:\.(?P<frac>\d*))?", "12.34")' -- --duplicate 100 'm.start("frac")'
Unpatched: Median +- std dev: 638 ns +- 22 ns
Patched: Median +- std dev: 576 ns +- 16 ns
$ ./python -m perf timeit -s 'import re; m = re.match(r"(?P<int>\d+)(?:\.(?P<frac>\d*))?", "12.34")' -- --duplicate 100 'm.group("frac")'
Unpatched: Median +- std dev: 1.15 us +- 0.03 us
Patched: Median +- std dev: 1.07 us +- 0.03 us |
Since _sre.compile is not a public API, and is called only with exact list/dict/tuple types in the stdlib, I prefer to ignore possible differences between base classes and subclasses. This makes the code cleaner. Searching on GitHub shows that usages of _sre.compile() in third-party code are not compatible with current _sre.compile() since all arguments now are mandatory and indexgroup must be a tuple, not None. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: