Add PyUnicodeWriter_WriteASCII() function

[vstinner]

Hi,

I propose adding the following function the Python 3.15 C API.

API: int PyUnicodeWriter_WriteASCII(PyUnicodeWriter *writer, const char *str, Py_ssize_t size)

Documentation:

Write the ASCII string str into writer.

size is the string length in bytes. If size is equal to -1, call strlen(str) to get the string length.

str must only contain ASCII characters. The behavior is undefined if str contains non-ASCII characters.

On success, return 0.
On error, set an exception, leave the writer unchanged, and return -1.

The function is up to 2.0x faster than PyUnicodeWriter_WriteUTF8().

There are 2 reasons to add this function:

• Get the same performance when updating code using the old private _PyUnicodeWriter API to use the new public PyUnicodeWriter API.
• Get the best performance for new code using the public PyUnicodeWriter API: make the API appealing in term of performance.

Background: Using the public PyUnicodeWriter C API made the json module slower.

Vote:

• @encukou
• @erlend-aasland
• @mdboom
• @serhiy-storchaka
• @vstinner
• @zooba

[zooba]

Seems reasonable to me.

I'd check with the RM about adding it to 3.14 as well. I suspect it should be okay - there's no compatibility impact, and it'll save introducing one by having to choose a different API between 3.14/3.15.

[encukou]

Can we name this PyUnicodeWriter_WriteASCII_Unchecked, to signal that something that's “usually” a ValueError will cause undefined behaviour?
A reviewer should pause when they see this call, and reviewing autocompletions is getting more and more common.

[malemburg]

The behavior is undefined if str contains non-ASCII characters.

Why does this have to be undefined ?

This could lead to data corruption or weird errors when such data is being processed by code which rightly assumes that an ASCII string is an ASCII string.

Since the data will have to be copied, the loop could check for this, no ?

[encukou]

data corruption or weird errors when such data is being processed

”Undefined behaviour” already means there are no guarantees on anything after the bad call. IMO, that's enough; we need to specifically document that this will create a broken object that can cause trouble later.

Since the data will have to be copied, the loop could check for this, no ?

That's a good point!
That's what PyUnicodeWriter_WriteUTF8 already does (except it can succeed for non-ASCII text, but that's good).

But apparently it's 2× slower. If it can be made fast, we don't need a new function. Let's make sure we can't make it fast.

It looks like the current function is optimized for long-ish text, not for static constants of 1-5 bytes.

I commented on the issue with a request to benchmark an overoptimized version, to see how fast this could get.

[zooba]

We'd make this fast by using memcpy or strcpy rather than a for loop with a condition in it. Those will use all sorts of tricks to make copying fast (e.g. DWORD (or bigger) at a time, aligned/unaligned start/finish, non-overlapping buffers, etc.), and while it's possible to also check for any char having MSB set, we'd have to write and maintain it ourselves.

Much much easier all around to offer an "unchecked" version. Debug builds could assert that the incoming string is all ASCII, but otherwise I have no problem leaving the responsibility on the caller. (Also no problem putting "unchecked" in the name - saves the reader having to wonder whether it's entirely safe or not.)

[malemburg]

Wouldn't it be better to add a parameter which enables checking ?

While some sources may already know that the data is ASCII (or voluntarily take the risk of breaking things badly) and could set this parameter, others may well want to invest the few extra cycles for extra checks.

[zooba]

I think it's sufficient to have a separate function. And actually better, since a parameter is probably going to read as 1 to someone looking at the code, whereas the function reads WriteASCII_Unchecked and is far clearer about what's going on. The documentation should encourage that it only be used for literals and not for untrusted inputs, and only when performance is critical.

The chance of choosing the wrong argument seems just as high as choosing the wrong function, and really, there's no harm in "accidentally" making a string from non-ASCII and then checking if you use the normal function.

[vstinner]

@encukou:

Can we name this PyUnicodeWriter_WriteASCII_Unchecked

I would prefer to remove the underscore: PyUnicodeWriter_WriteASCIIUnchecked(). We already have PyThreadState_GetUnchecked() which has no underscore before Unchecked.

For this function, it's not about checking the return value, but more than passing non-ASCII in unsafe. Maybe PyUnicodeWriter_WriteASCIIUnsafe() would be a better name.

@encukou:

But apparently it's 2× slower. If it can be made fast, we don't need a new function. Let's make sure we can't make it fast.

Python ASCII and UTF-8 decoders are already highly optimized. Maybe we can make it a little bit more efficient, but it will always be slower than the most efficient memcpy() function used by PyUnicodeWriter_WriteASCIIUnchecked().

@malemburg:

Wouldn't it be better to add a parameter which enables checking ?

If you want to check for non-ASCII characters, just use PyUnicodeWriter_WriteUTF8().

PyUnicodeWriter_WriteASCIIUnchecked() works well with ASCII literal strings. If you're unsure if the argument is ASCII, use PyUnicodeWriter_WriteUTF8().

[malemburg]

PyUnicodeWriter_WriteASCIIUnchecked() works well with ASCII literal strings. If you're unsure if the argument is ASCII, use PyUnicodeWriter_WriteUTF8().

Fair enough. This ought to be mentioned in the corresponding docs, then.

[encukou]

+1 to PyUnicodeWriter_WriteASCIIUnchecked. Indeed, it's hard to beat memcpy.

It would be nice to speed up PyUnicodeWriter_WriteUTF8, but, ASCII-only literals are an important use case, and this is the right tool for that.

[serhiy-storchaka]

I had doubts about this function because there have been objections about C APIs with undefined behavior recently, so I wanted to get some opinions on this issue.

[vstinner]

Ping @mdboom.

[vstinner]

The C API Working Group voted in favor of adding the function, thanks. I close the issue.