Add `PyDict_FromItems()` function

[vstinner]

Hi,

See the Proposed API document.

Update: I replaced the issue description with a link to the proposed API.

[encukou]

I commented on the PR.

[vstinner]

@encukou would prefer to have a single function:

PyObject* PyDict_FromItems(
    PyObject *const *keys,
    Py_ssize_t keys_offset,
    PyObject *const *values,
    Py_ssize_t values_offset,
    Py_ssize_t length)

For example, PyDict_FromItems(items, length) becomes:

PyDict_FromItems(items, 2, items + 1, 2, length)

IMO this API is more complicated to use and more error-prone: there is a higher risk to misuse it and get bugs.

What do other people think about these APIs?

[encukou]

Preferably with arguments named stride rather than offset.

This:

• is a single function (this is a factor since we're setting a possible precedent for an Update API)
• also gives you fromkeys() (set values_stride to 0)
• allows keys/values pointers to be in an array of more complicated structures. (I don't find this one too convincing.)

[davidhewitt]

As per the PRs, I think I want the single function with strides / offsets too 😄

[vstinner]

Ok, let me open a vote for this API:

PyObject* PyDict_FromItems(
    PyObject *const *keys,
    Py_ssize_t keys_stride,
    PyObject *const *values,
    Py_ssize_t values_stride,
    Py_ssize_t length)

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

[zooba]

I'm not a fan of encouraging users to make native arrays of Python objects - it puts too much burden for reference count management, especially in the case of failure halfway through creating the array, or cleaning up the arrays after failure of the _FromItems() function.

A 13% performance improvement feels very low-level to me. I don't have an issue with the low-level API, and I do have an issue with the complexity of the proposal.

What about an API that can be applied to an existing dict to rebucket at a certain size, and a set of constant flags for assuming things (such as the key type). Would be used as:

d = PyDict_New();

// insert this line for ~10% improvement but no functionality change
PyDict_SetAssumptions(my_data.size(), PY_DICTFLAG_UNICODE_KEYS);

for (auto &i : my_data) {
    PyDict_SetItem(...);
}

That extra line could be omitted and everything still works, which means users can write correct/working code first and then add it in as an optimisation later on. When applied to an empty dict, that call should be ~free (same cost as bundling it into the _New), and it also makes sense to apply to a non-empty dict. That feels like a better development workflow than trying to construct arrays of Python objects.

I don't remember whether _SetItem with a non-str key fails or de-optimizes if it's assuming str keys, but either way, I think it's fine. I'm 99.9% certain that it won't rebucket when adding and there's still space, and that it will rebucket if you later exceed that, but it might be more sensible to have a "minimum size" (internal) property so we don't resize below that once a caller has set it.

[encukou]

This discussion is going in circles now. It's probably time for a PEP.

[zooba]

Sorry, I missed the rest of the circle.

Where did people rule out modifying an existing dict, and where did they rule out using flags for the Unicode keys argument instead of a non-extensible boolean?

[vstinner]

A 13% performance improvement feels very low-level to me. I don't have an issue with the low-level API, and I do have an issue with the complexity of the proposal.

The initial issue is that the private _PyDict_NewPresized() function is used by 3rd party projects and I would like to get rid of private functions. The question is how to replace it with a public function:

• Add PyDict_NewPresized() function #80 proposes PyObject* PyDict_NewPresized(Py_ssize_t size, int unicode_keys)
• this issue proposes PyObject* PyDict_FromItems(PyObject *const *keys, ..., Py_ssize_t length)

@encukou doesn't like PyDict_NewPresized() because int unicode_keys exposes an implementation detail.

Where did people rule out modifying an existing dict, and where did they rule out using flags for the Unicode keys argument instead of a non-extensible boolean?

PyDict_SetAssumptions() is very close to PyDict_NewPresized(). I'm not convinced that replacing int unicode_keys with PY_DICTFLAG_UNICODE_KEYS makes a big difference here. PyDict_FromItems() doesn't expose unicode_keys/PY_DICTFLAG_UNICODE_KEYS: instead, it does scan keys to decide what's the best internal format for keys.

[vstinner]

This discussion is going in circles now. It's probably time for a PEP.

I don't want to write a PEP and involve the Steering Council for a single function. Instead, I wrote down a PEP-like document at: capi-workgroup/api-evolution#55.

[zooba]

PyDict_SetAssumptions() is very close to PyDict_NewPresized()

Yes, that's intentional, because I think _NewPresized is the better API here (compared to making users create native arrays of refcounted objects). A fully worked out example using _NewPresized or _SetAssumptions¹ is going to be less code and more robust than FromItems, because the caller doesn't have to handle deallocating/decrefing the array on failure.

I'm not convinced that replacing int unicode_keys with PY_DICTFLAG_UNICODE_KEYS makes a big difference here.

It doesn't make a difference here - it makes a difference when we add (say) int keys, and then we don't have to deprecate the API and create a new one - we just add a new flag. But I'm also okay with not allowing users to set it at all, though some clearly think it gives them a benefit and so will try and figure out ways to get that benefit.

it does scan keys to decide what's the best internal format for keys.

Don't worry, I didn't miss that. But we're talking about a relatively low-level optimization here, so it's okay to expect the caller to have done that (or more likely, since they're creating the dict, they already know for sure what kind of keys they're going to have). So it'll be even faster in the case where it doesn't have to scan.

Footnotes

1. Which, to be clear, I don't love that name. PyDict_Presize or _Preallocate might be better, but I care less about the name than the usage. ↩

[vstinner]

A 13% performance improvement feels very low-level to me.

By the way, PyDict_FromItems() is around 1.12x faster than PyDict_New()+PyDict_SetItem(), whereas PyDict_NewPresized()+PyDict_SetItem() is "only" around 1.05x faster than PyDict_New()+PyDict_SetItem(): see the benchmark.

Only on the performance side, PyDict_FromItems() is more appealing than PyDict_NewPresized() / PyDict_SetAssumptions().