If we're still assuming a long object,. we can just usePyLong_AsInt instead. We don't care about an overflow as we're only interested in values in [0, 255].

Thanks. I copied this code from bytearray but you're absolutely right thatPyLong_AsInt is a much better fit here.

An existing test with-sys.maxsize failed withPyLong_AsInt. A fix would be to check forOverflowError and reraiseValueError instead, but I think it's easier to simply revert toPyLong_AsLongAndOverflow and let the range check that follows raiseValueError when out of the byte range.

Previously, we usedPyNumber_AsSsize_t (which invokes_PyNumber_Index, hence possibly creating side effects). This will be a small behavioral change. While I understand Guido's comment, I'm wondering whether we should keep the old behaviour (though I don't know how it could be useful in production, namely makingbytes() have side-effects on lists if one of their element being converted invokes__index__).

Note that the current code also prevented crashes by temporarily increfingitem before callingPyNumber_AsSsize_t.

Since this PR is not supposed to be about altering behaviors, I'll keep the current behavior for this PR and file a separate bug against the current behavior then. It should be considered a bug because like Guido pointed out, a list can potentially be mutated inside__index__, resulting in the item copy loop accessing freed memory. Despite the unlikelihood that such code exists in the real world, it can still potentially happen and cause a crash.

You don't have a crash because the list size is checked at every iteration (though what should be checked perhaps is that the list pointer is not NULL). We incref the item before calling__index__ on it so it shouldn't cause crashes. INCREFing before is a trick you also use for use-after-free issues and evil mutations (but we can easily check if this is crashing or not as follows:

classEvilInt:def__index__(self):x.clear()return0x= [1,2,EvilInt(),4]bytes(x)

and this does not crash.

Ahh now I see what INCREF and DECREF are doing there. I'll add them back in a bit too then. Thanks.

Can we have the pyperf benchmarks on the PR as well? (namely, the nice table with two columns and the diffs as well as the benchmark script? thanks)

IIRC, NEWS should not contain an empty line.

Previously, we usedPyNumber_AsSsize_t (which invokes_PyNumber_Index, hence possibly creating side effects). This will be a small behavioral change. While I understand Guido's comment, I'm wondering whether we should keep the old behaviour (though I don't know how it could be useful in production, namely makingbytes() have side-effects on lists if one of their element being converted invokes__index__).

Note that the current code also prevented crashes by temporarily increfingitem before callingPyNumber_AsSsize_t.

If we're going for performance, then we can do even better.PySequence_Fast_ITEMS will callPyList_Check, but we already know that it's exactly a list or tuple here.

But all we know is that it is either a list or a tuple, but we still don't know which of the two it is, so aPyList_Check call is still in order.

Right, butPyList_Check is extra work because that will check for a subclass. It's not really going to be noticable, but it's something to think about :)

Alternatively, you can just duplicate the functions as we did previously. This is not really an issue IMO, and we couold use the fact that tuples are immutables for instance to avoid INCREF/DECREF values.

You need to acquire the critical section before callingPySequence_Fast_ITEMS andPySequence_Fast_GET_SIZE. Attempting to read mutable data without a lock isn't thread safe.

PyLong_CheckExact probably fits better here, and is faster!

(The previous code wasn't doing an exact check so we shouldn't change it)

Well, it wouldn't be a breaking change, just a performance loss for the (very niche!) set of cases that use special ints. I'm also slightly worried that non-exact ints might have some nasty side effects that we aren't anticipating here (e.g. can they mess with thePy_ssize_t value?)

They can mess withPy_ssize_t values but only through__index__ and we already check that withPyNumber_AsSsize_t, but the problem is wider. For example,np.int32() values can be passed tobytes([...]) but without this check, we will impact performances of numpy-related code which is something we don't want to.

Is casting to bytes a common thing to do with numpy integers, or is that speculation? (I see your point, I'm just sort of gauging what the cost-benefit would be here.)

I'd say yes, if we're considering serialization or introspection. I imagine that we can have such things when people are working with images because their arrays won't necessarily be pure Python lists butnp.ndarray objects which may have non-primitive data types. I'm not aware of a wild opensource usage though but IMO, since we want to improve performance overall, we shouldn't penalize existing users. If we can slightly decrease our performance gain so to have a stable result, it's good.

Nonetheless, if we want to fallback to a slow path for non-exact ints, benchmarks should show how performances are impacted (a simpletimeit could be sufficient but I'd be more comfortable with a much precise benchmark).

Instead of these goto tricks, consider instead to pull out the critical section code into a helper function; it should result in cleaner and more readable code.

To avoid gotos in I defined a helperPy_EXIT_CRITICAL_SECTION in Include/internal/pycore_critical_section.h to be able to return at errors. Note: the PR has not been merged yet, so I am not sure this is the way to go.

Ah yes, refactoring into a helper would definitely be cleaner and we wouldn't the comment.

Btw, I think we could have those macro helpers because they would help us in those situations.

To avoid gotos in I defined a helperPy_EXIT_CRITICAL_SECTION in Include/internal/pycore_critical_section.h to be able to return at errors. Note: the PR has not been merged yet, so I am not sure this is the way to go.

I'm not sure about introducing such an API. IMO, it is better to keep the critical section APIs dead simple. If you need to avoid gotos, refactor.

Yeah, I agree. With critical sections the safest way to go is to create afoo_lock_held function, or use AC's@critical_section if applicable. Wereally don't want cases where a critical section isn't popped off at the end.