This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author vstinner
Recipients Mark.Shannon, jdemeyer, josh.r, malin, methane, vstinner
Date 2019-08-13.15:37:15
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1565710635.75.0.509899777639.issue37774@roundup.psfhosted.org>
In-reply-to
Content
> We're not talking about prefetching here. The Py_LIKELY/Py_UNLIKELY macros only affect which of the two code paths in a branch is the "default" one, i.e. the one not involving a jmp.

I know. My point about prefetching is that if we provide a way to developers to tune the compiler, they will use it, but it can lead the a performance regression whereas not using these tools would lead to such issue.

> I claim that adding Py_LIKELY/Py_UNLIKELY will reduce the performance randomness of non-PGO builds. So it would be strange to use that randomness as an argument *against* this patch.

It's right that Py_LIKELY/Py_UNLIKELY should make performance more reliable. My question is if it is safe to let developers "abuse" it. If these macros are misused, they can lead to a performance regression.

I'm not sure that Py_LIKELY/Py_UNLIKELY is always more performant than not using it. For example, if you mark the error path as unlikely: this path will become slower. If the error path is taken "frequently" enough, the slowdown can become significant overall.

It's tricky to decide if a code path is likely or not.

I would be ok to use Py_UNLIKELY for sanity checks which raise SystemError: these code paths must never be taken in a valid code. I'm more in favor of simply removing these runtime checks in release mode, but that's a different issue :-) (bpo-37406)

I'm not sure about judging if the error case raising an exception really deserve a Py_UNLIKELY.

Maybe I would be more comfortable if you would split the PR into multiple parts. One PR to add the macros. Then one PR per kind of modified tests.

For example, "if (Py_UNLIKELY(tstate->use_tracing))" in ceval.c sounds reasonable, but I would prefer to have the opinion of other core devs.

About the macros, I would prefer to make it private to not promote it. It should also have a good explanation why it's better to not use it, except if you really well understand what you do.
History
Date User Action Args
2019-08-13 15:37:15vstinnersetrecipients: + vstinner, methane, Mark.Shannon, jdemeyer, josh.r, malin
2019-08-13 15:37:15vstinnersetmessageid: <1565710635.75.0.509899777639.issue37774@roundup.psfhosted.org>
2019-08-13 15:37:15vstinnerlinkissue37774 messages
2019-08-13 15:37:15vstinnercreate