Speedup posixpath.abspath() for relative paths

Feature or enhancement

Proposal:

When normalising a relative path, we don't need to process the cwd as it's already normalised, which could get expensive if it's long:

< _Py_normpath_and_size(wchar_t *path, Py_ssize_t size, Py_ssize_t *normsize)
---
> _Py_normpath_and_size(wchar_t *path, Py_ssize_t size, Py_ssize_t start, Py_ssize_t *normsize)
2397a2398,2402
>         return path;
>     }
>     // Start beyond end of path
>     if (start >= size) {
>         *normsize = size;
2457a2463,2467
>     // Skip past cwd
>     if (path + start > p1) {
>         p1 = p2 = path + start;
>         lastC = *(p1-1);
>     }
2525c2535
<     return _Py_normpath_and_size(path, size, &norm_length);
---
>     return _Py_normpath_and_size(path, size, 0, &norm_length);

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:

No response

Linked PRs

gh-117587: Add C implementation of os.path.abspath #117855

The idea is working, but I don't know what to do now:

/*[clinic input]
os._path_abspath

    path: object
    start: Py_ssize_t=0

Make path absolute.
[clinic start generated code]*/

static PyObject *
os__path_abspath_impl(PyObject *module, PyObject *path, Py_ssize_t start)
/*[clinic end generated code: output=69e536dbe18ecf3a input=29df0995bc21a9cf]*/
{
    if (!PyUnicode_Check(path)) {
        PyErr_Format(PyExc_TypeError, "expected 'str', not '%.200s'",
            Py_TYPE(path)->tp_name);
        return NULL;
    }
    Py_ssize_t len;
    wchar_t *buffer = PyUnicode_AsWideCharString(path, &len);
    if (!buffer) {
        return NULL;
    }
    Py_ssize_t abs_len;
    wchar_t *abs_path = _Py_normpath_and_size(buffer, len, start, &abs_len);
    PyObject *result = PyUnicode_FromWideChar(abs_path, abs_len);
    PyMem_Free(buffer);
    return result;
}

The idea is working, but I don't know what to do now:

Work on getting posixpath.abspath() to work with the new builtin function, with all tests passing. If the performance is good enough, you could just rename the new builtin as _path_normpath_with_start(). Otherwise, implement it all in _path_abspath().

@barneygale

Reading through the posts in the issue, it is not clear to me if there is significant core dev support for this idea. It would surprise me if abspath() was used for hot code. The diff of the linked PR is considerable. Does the added complexity outweigh the cost of improving the performance of an API like abspath()?

@barneygale and @serhiy-storchaka: thoughts?

I reckon it unlikely that abspath() is a performance bottleneck in anyone's code.

It's a slightly dangerous function too - it calls normpath() which can change the meaning of a path like symlink/... It would be a shame to double down on that behaviour by implementing it in C. OTOH I can't see any reasonable path towards fixing abspath().

@eryksun

The diff of the linked PR is considerable.

Note that I also extended the capabilities of _Py_normpath_and_size(). We could expose the start and explicit_curdir parameters, but I want to discuss that later:

You can use start to faster normalise a path after joining with an already normalised path:

result = join(path1, path2)
result = normpath(result, start=len(result) - len(path2))

And explicit_curdir helps to avoid ambiguity: ./C:foo has a vastly different meaning than C:foo.

And @eryksun asked specifically to implement abspath() in C.

I reckon it unlikely that abspath() is a performance bottleneck in anyone's code.

I just don't like the fact that we're normalising the cwd which is already normalised. Otherwise I definitely wouldn't have implemented it in C (there are other functions which we would benefit more of). And making it faster certainly doesn't hurt, right? I think it's even faster now that I got rid of the Python wrapper.

@eryksun

And @eryksun asked specifically to implement abspath() in C.

We've implemented enough of the implementation in C in terms of normpath() and splitroot() that it becomes feasible. But if you want to expose a private _path_normpath_ex() that supports start and explicit_curdir, we could make use of that instead, and continue to implement abspath() itself in Python. (I'm not satisfied with the direction you've been pushed into with the implementation of the function in C, and backing off on that saves me the time of trying to implement a rewrite that would make everyone happy.)

When adding new code, we try hard to weigh a lot of different factors: performance, usefulness, maintainability, readability, code complexity, the number of lines added or subtracted, and backwards compatibility, to name a few. We also try to understand any opposition to a change; "why is my PR/issue being criticised or rejected?"

I do not think this change is worth it. Performance optimisations are nice, but only if the benefits outweigh the added cost. Adding a C implementation of existing Python code needs a really good reason. C code has a lot more maintenance overhead than Python code. If the added C code is already in a state where it needs refactoring, you're directly adding technical dept.

Moreover, there has been talk about rewriting C code in Python when the interpreter has become fast enough¹. This should also be taken into account when considering to add C code.

https://github.com/faster-cpython/ideas/issues/91 ↩

I'll leave it for Barney to decide if this issue should be kept open or if it should be closed.

@nineteendo

Sorry @nineteendo, I don't think the C implementation is worth it on balance, for the same reasons as Erlend gave.

@barneygale

@barneygale, what about implementing _path_normpath_ex() that supports start and explicit_curdir? Supporting explicit_curdir is important for Windows to avoid problems with DOS device names (e.g. "./con") and named streams on single-letter filenames (e.g. "/C:spam"). Supporting start is an improvement for POSIX to avoid having to re-normalize the already-normalized current directory.

Hmm, start is not a great API as it doesn't work well for bytes. normpath('foo', 'C:/bar') would be better, but would be difficult to implement on Windows. I'll make a new pull request just to implement explicit_curdir.

I'll make a new pull request just to implement explicit_curdir.

No; please do not pollute the already too long list of open PRs. Discuss any possible change in an actionable issue first. If you need to open an experimental PR, do so on your own fork. The CPython repo is not the place for experimentation.

Discuss any possible change in an actionable issue first.

I have already done that: #119826

Navigation Menu

Repository navigation

Speedup `posixpath.abspath()` for relative paths #117587

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Speedup posixpath.abspath() for relative paths #117587

Description

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Activity

nineteendo commented on Apr 12, 2024

eryksun commented on Apr 12, 2024

nineteendo commented on Apr 13, 2024

erlend-aasland commented on May 30, 2024

barneygale commented on May 30, 2024

nineteendo commented on May 30, 2024

eryksun commented on May 30, 2024

erlend-aasland commented on May 30, 2024

Footnotes

erlend-aasland commented on May 30, 2024

barneygale commented on May 30, 2024

eryksun commented on May 30, 2024

nineteendo commented on May 31, 2024

erlend-aasland commented on May 31, 2024

nineteendo commented on May 31, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Speedup `posixpath.abspath()` for relative paths #117587