New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Py_MEMCPY: Use memcpy on Windows? #72313
Comments
Py_MEMCPY() has a special case for small blocks on Windows to work around an ancient performance issue in MSVC. Can we safely assume that recent MSVC properly optimize memcpy()? See bpo-28055 /* Py_MEMCPY can be used instead of memcpy in cases where the copied blocks
#if defined(_MSC_VER)
#define Py_MEMCPY(target, source, length) do { \
size_t i_, n_ = (length); \
char *t_ = (void*) (target); \
const char *s_ = (void*) (source); \
if (n_ >= 16) \
memcpy(t_, s_, n_); \
else \
for (i_ = 0; i_ < n_; i_++) \
t_[i_] = s_[i_]; \
} while (0)
#else
#define Py_MEMCPY memcpy
#endif |
Yes, memcpy (or preferably memcpy_s, which includes the size of the destination) are basically intrinsics that will inline short copies and call out to a range of implementations depending on alignment/overlap/etc. |
Perfect, thanks! Should we keep the Py_MEMCPY() macro for now and define it to memcpy()? Or should I get rid of it and just define it for b/w compatibility? |
I'm okay with removing it completely, if you're willing to change that much code. (Most of my additions already uses memcpy or memcpy_s directly, since I wasn't even aware of this macro :) ) |
Behold the power of Unix! :) $ find -name '*.[ch]' | xargs sed -i s/Py_MEMCPY/memcpy/g Victor, are you fine with the change? I'm going to keep Py_MEMCPY around. |
The macro looks public, you cannot remove it. Make it an alias to memcpy(), but explain that it's only kept for backward |
As I said, I'm going to keep Py_MEMCPY around but replace it everywhere else. /* Py_MEMCPY is kept for backwards compatibility,
* see https://bugs.python.org/issue28126 */
#define Py_MEMCPY memcpy |
New changeset bedce61ae0a0 by Christian Heimes in branch '3.6': New changeset f5d32ed0f9c2 by Christian Heimes in branch 'default': |
Shouldn't the NEWS entity be in the C API section? |
Isn't the C API section reserved for C API changes? I haven't changed the C API but rather optimized the core. |
The Py_MEMCPY name doesn't start by an underscore, and it is not inside the "#ifndef Py_LIMITED_API" block. But it is not documented either. I don't know what is the status of this macro. If this is a part of public API, this change should be documented in the C API section. If it is inner macro, this change shouldn't be mentioned in Misc/NEWS. |
I don't think that it's very useful to discuss the status of the |
My change doesn't break or change 3rd party application. They might get a tiny bit faster on Windows, if they have used Py_MEMCPY() for up to 16 bytes. That's it. |
Even that's unlikely as the loop in the macro would have been unrolled in practically every case. This is about removing an unnecessary macro. Certainly no API change unless we delete it entirely (which I'm okay with), and IMHO if it has an issue number it should go in NEWS. Maybe build is the right section? |
Py_MEMCPY() has a special case for small blocks on Windows to work around an ancient performance issue in MSVC. Can we safely assume that recent MSVC properly optimize memcpy()? See bpo-28055 /* Py_MEMCPY can be used instead of memcpy in cases where the copied blocks
#if defined(_MSC_VER)
#define Py_MEMCPY(target, source, length) do { \
size_t i_, n_ = (length); \
char *t_ = (void*) (target); \
const char *s_ = (void*) (source); \
if (n_ >= 16) \
memcpy(t_, s_, n_); \
else \
for (i_ = 0; i_ < n_; i_++) \
t_[i_] = s_[i_]; \
} while (0)
#else
#define Py_MEMCPY memcpy
#endif |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: