Remove _get_[both]sep[s]() calls in os.path

[nineteendo]

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 sep

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

• gh-117349: Speedup os.path #117610

Linked PRs

• gh-117634: Remove _get_[both]sep[s]() calls in os.path #117635

[erlend-aasland]

Python function calls are always expensive.

This is being worked on:

• Speeding up Python-to-Python calls. faster-cpython/ideas#661

[nineteendo]

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:

-seps = _get_bothseps(p)
+seps = b'\\/' if isinstance(p, bytes) else '\\/'

[terryjreedy]

Why do you think that the isinstance call faster than the _get_bothseps call? Removing a redundant call is another matter.

[eryksun]

@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]

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.

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

	while i < n and path[i] not in _get_bothseps(path):
	i += 1

Doing that makes it 61% faster for long user names (>100 characters).

[erlend-aasland]

I'm not sure the performance win outweights the increase in code duplication.

[nineteendo]

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]

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]

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]

I've made a separate issue for ntpath.expanduser(): #117686 We really shouldn't call _get_bothseps() in every iteration.

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.

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.

That's great; thanks for helping reducing our backlog!