Repository navigation
Remove _get_[both]sep[s]() calls in os.path #117634
Description
Feature or enhancement
Proposal:
Python function calls are always expensive. We can replace all _get_sep() & _get_bothseps() calls in os.path with ternary operators, merging with existing isinstance() checks where applicable. e.g:
-seps = _get_bothseps(p)
+seps = b'\\/' if isinstance(p, bytes) else '\\/'Also, in posixpath.expanduser() we can replace root with the already assigned sep from earlier:
if isinstance(path, bytes):
userhome = os.fsencode(userhome)
- root = b'/'
-else:
- root = '/'
-userhome = userhome.rstrip(root)
-return (userhome + path[i:]) or root
+userhome = userhome.rstrip(sep)
+return (userhome + path[i:]) or sepHas this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Activity
erlend-aasland commented on Apr 8, 2024
This is being worked on:
nineteendo commented on Apr 8, 2024
OK, but calling a function will always have some overhead, we can never fully eliminate that (unless we add macro's to python).
And the code will still fit on a single line, making the intention much clearer:
terryjreedy commented on Apr 8, 2024
Why do you think that the isinstance call faster than the _get_bothseps call? Removing a redundant call is another matter.
eryksun commented on Apr 8, 2024
@terryjreedy, _get_bothseps() calls isinstance(). The suggestion is to manually inline the code in the source at each call site since Python's compiler doesn't support inline function calls. It could be considered unnecessary optimization, however. Call overhead of a few nanoseconds is a drop in the bucket compared to work that requires a few microseconds.
nineteendo commented on Apr 9, 2024
In my benchmarks, the new code is 4-12% faster: #117635 (comment). If you want benchmarks for longer input, let me know.
Also, ntpath.expanduser() calls this function in every iteration of a loop, without saving the result:
cpython/Lib/ntpath.py
Lines 377 to 378 in e338e1a
Doing that makes it 61% faster for long user names (>100 characters).
erlend-aasland commented on Apr 9, 2024
I'm not sure the performance win outweights the increase in code duplication.
nineteendo commented on Apr 9, 2024
Do I need to make a separate issue for os.path.expanduser() (on posix we're doing a redundant isinstance() check) or can I also change the other functions?
erlend-aasland commented on Apr 9, 2024
As I've said earlier, micro-optimisations are generally hard to get support for; we mostly prefer code clarity, readability and maintainability over performance.
I would instead encourage you to direct your enthusiasm towards the existing issues in the bug-tracker. There's close to 7000 issues in the tracker. What we really need help with, is triaging this heap of issues, fixing those who can be fixed, closing those who are invalid, etc. I think you could be of real help (and you would experience more support from the core team) if you directed your skills towards the bug-tracker.
nineteendo commented on Apr 9, 2024
I've made a separate issue for ntpath.expanduser(): #117686 We really shouldn't call _get_bothseps() in every iteration.
I've also created 5 pull requests for existing issues in the issue tracker: https://github.com/python/cpython/pulls/nineteendo.
So if you want, you can review them.
erlend-aasland commented on Apr 9, 2024
That's seems a fair point. I suggest to close this issue, though; IMO the performance gains do not outweigh the added code duplication nor the churn.
That's great; thanks for helping reducing our backlog!