Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)

@tim-one

BPO	37812
Nosy	@tim-one, @rhettinger, @mdickinson, @vstinner, @animalize, @gnprice, @sir-sigurd, @aeros
PRs	bpo-37812: Make the small-int alloc optimization unconditional. #15203 bpo-37812: Expand confusing CHECK_SMALL_INT so return is explicit. #15216 bpo-37812: Expand CHECK_BINOP to make an (almost) explicit return. #15448

^{Note: 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:

assignee = None
closed_at = None
created_at = <Date 2019-08-10.21:07:30.736>
labels = ['interpreter-core', 'type-feature', '3.9']
title = 'Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT)'
updated_at = <Date 2019-09-22.06:41:33.865>
user = 'https://github.com/gnprice'

bugs.python.org fields:

activity = <Date 2019-09-22.06:41:33.865>
actor = 'aeros'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2019-08-10.21:07:30.736>
creator = 'Greg Price'
dependencies = []
files = []
hgrepos = []
issue_num = 37812
keywords = ['patch']
message_count = 36.0
messages = ['349359', '349360', '349376', '349377', '349379', '349414', '349418', '349420', '349425', '349429', '349430', '350352', '350353', '350378', '350380', '350398', '350402', '352670', '352685', '352688', '352689', '352690', '352691', '352693', '352694', '352695', '352709', '352781', '352833', '352834', '352836', '352837', '352838', '352842', '352904', '352963']
nosy_count = 8.0
nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'vstinner', 'malin', 'Greg Price', 'sir-sigurd', 'aeros']
pr_nums = ['15203', '15216', '15448']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue37812'
versions = ['Python 3.9']

In longobject.c we have the following usage a few times:

PyObject *
PyLong_FromLong(long ival)
{
    PyLongObject *v;
    // ... more locals ...

CHECK_SMALL_INT(ival);

    if (ival < 0) {
        /* negate: can't write this as abs_ival = -ival since that
           invokes undefined behaviour when ival is LONG_MIN */
        abs_ival = 0U-(unsigned long)ival;
        sign = -1;
    }
    else {
    // ... etc. etc.

The CHECK_SMALL_INT macro contains a return, so the function can actually return before it reaches any of the other code.

#define CHECK_SMALL_INT(ival) \
    do if (-NSMALLNEGINTS <= ival && ival < NSMALLPOSINTS) { \
        return get_small_int((sdigit)ival); \
    } while(0)

That's not even an error condition -- in fact it's the fast, hopefully reasonably-common, path.

An implicit return like this is pretty surprising for the reader. And it only takes one more line (plus a close-brace) to make it explicit:

    if (IS_SMALL_INT(ival)) {
        return get_small_int((sdigit)ival);
    }

so that seems like a much better trade.

Patch written, will post shortly.

I've just sent #59408 which is the first of two patches for this. It's quite small.

The second patch, which completes the change, is also small:
gnprice@c6b905104

It depends on the first one, so I think the easiest is for me to send it as a PR after the first is merged? Happy to do another workflow if that'd make review easier. (I don't see guidance in the devguide on handling a sequence of patches, so I'm making this up.)

Thanks for the contribution Greg. I understand your reasoning but am going to decline PR 15203. Though the feature is not experimentnal, it is something we want be able to modify, shut-off, or extend at build time. Sometimes for testing we turn it off in order to identify identity test bugs. Also, eveonline was controlling this to save memory.

P.S. Making the returns explicit is a reasonable change to make, but the core #ifdef logic should remain.

Sometimes for testing we turn it off in order to identify identity test
bugs.

Interesting! Well, if the option is useful for testing, that's certainly a
good reason to keep it.

Also, eveonline was controlling this to save memory.

Also interesting. I feel like there must be something about their setup
that I'm not imagining, because this is a pretty tiny amount of memory (9
kB) on the scale of a typical Python server process in my experience.

Or do you mean they were increasing the numbers? One thought I had looking
at this was actually that it would be interesting to do some measurements
and try to pick new (default) values for these parameters - it looks like
they're the same today as in 1993, and it's likely that with all that's
changed in typical hardware since then (plus the switch from Python ints +
longs to only what we used to call longs), the optimum values today are
different from the optimum in 1993. And certainly one of the effects of
this optimization when its hit rate is good is reducing memory consumption

you have only one 17 instead of however many you make.

I'll rework the explicit-return patch to send without this change. It'll be
a little trickier but should be entirely doable.

On Sat, Aug 10, 2019, 23:19 Raymond Hettinger <[email protected]>
wrote:

Raymond Hettinger <[email protected]> added the comment:

P.S. Making the returns explicit is a reasonable change to make, but the
core #ifdef logic should remain.

----------

Python tracker <[email protected]>
<https://bugs.python.org/issue37812\>

I'll rework the explicit-return patch to send without this
change. It'll be a little trickier but should be entirely doable.

An explicit return would be a nice improvement.

On the other hand, if it is tricky and requires something more than minor surgery, that would be a hint that it isn't worth it. There is some value in code that is stable and known to be working just fine. See: https://en.wikipedia.org/wiki/There_are_known_knowns

On the other hand, if it is tricky and requires something more than minor surgery, that would be a hint that it isn't worth it. There is some value in code that is stable and known to be working just fine.

Definitely true!

I think this change turned out quite clean, though. Just sent, as #59421 -- please take a look.

How about using a hybrid implementation for int.

For example, on 64-bit platform, if (a integer >=-9223372036854775808 and <=9223372036854775807), use a native signed long to represent it.

People mostly use +-* operations, maybe using native int is faster, even including the cost of overflow checking.

If operation will overflow or other operation like **, we can transform native int to current form and run in current code path.

Ma Lin: Everything in Python is an object, nothing is native. There is room to reconsider the implementation of integer objects, known internally as PyLong objects. If you want to tease-out an alternative implementation, please do so on the python-ideas maillist. Small integer operations (including) indexing used to be smaller and faster in Python 2.7, so there is likely some room for improvement on what we have now.

I sent twice, but it doesn't appear in Python-Ideas list.
I will try to post to Python-Dev tomorrow.

python-dev likely isn't the right place. That is a forum for more mature ideas.

Thanks, Raymond, for the review on #59421!

Shortly after posting this issue, I noticed a very similar story in CHECK_BINOP. I've just posted #59653 to similarly make returns explicit there. It basically consists of a number of repetitions of

-    CHECK_BINOP(self, other);
+    if (!PyLong_Check(self) || !PyLong_Check(other)) {
+        Py_RETURN_NOTIMPLEMENTED;
+    }

with the names self and other varying from site to site. Though the expanded version isn't literally a return statement, I think it functions almost as well as one for the purposes that explicitness serves here, and much better so than a reference to CHECK_BINOP.

I agree that both changes should be reverted.

There is another commit after the two commits:
c6734ee

It is troublesome to revert them.

PR 16146 is on-going, maybe we can request the author to replace Py_UNREACHABLE() with assert(0).

We're wasted a lot of dev time on something that never promised much value in the first place. So, I'll revert and close this tracker issue

OK, so be it. I'll certainly agree that this series of threads consumed a lot more time than I anticipated or hoped, though I think we have different analyses of why that was.

(As you can probably guess, my previous message crossed with yours.)

Greg, would you be willing to work on a PR that reverts pr15216 and pr15710 while preserving the efforts of pr15192 ?

Raymond:

We're wasted a lot of dev time on something that never promised much value in the first place.

I'm one of the first to advocate to replace ugly macros with clean static inline functions. Macros are evil and can be too easily misused.

On PR 15216, Benjamin and Raymond advised to replace the macro with a function.

I'm also a believer that one day, we could replace duplicated C functions accepting variant of C "integers" types with a single larger type:

signed: replace (char, short, int, long, long long, ssize_t) with intmax_t
unsigned: replace (unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long, size_t) with uintmax_t

Sadly, it seems like such change has a small performance loss, and so cannot be done in "performance critical" code. I consider that longobject.c is such performance critical code.

--

I converted some macros of the Python C API to static inline functions, like Py_INCREF() or _PyObject_GC_TRACK():
https://vstinner.github.io/split-include-directory-python38.html

Compared to macros, static inline functions have multiple advantages:

Parameter types and return type are well defined;
They don't have issues specific to macros: see GCC Macro Pitfals https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html ;
Variables have a well defined local scope ;
GDB is able to retrieve then name when read the current C stack ("backtrace").

--

Again, I advice to add a small comment in longobject.c macros to explain that converting them to functions would loose performances.

When small integer are disabled at compilation time (NSMALLPOSINTS=0 and a NSMALLNEGINTS=0), I suggest to remove code related to small integers. IMHO CHECK_SMALL_INT() macro is a good practical solution for that. So I suggest to restore this macro.

I'm one of the first to advocate to replace ugly macros with clean static inline functions. Macros are evil and can be too easily misused.

As someone who has only more recently started learning the C-API (and C in general), I'm certainly in favor of replacing macros with functions when possible. I might be a bit biased, but it definitely makes the code a lot easier to understand (especially the macros with implicit returns). As long as there's not a significant performance loss and it doesn't introduce new complications, I don't see an issue with it.

From my understanding, readability isn't as high of a priority in the C code, but certainly there's some benefit to improving areas that are more difficult to understand. The easier the code is to understand, the lower the overall maintenance cost becomes.

Of course, this should certainly be done in moderation to reduce the introduction of unnecessary new bugs and the cost in reviewer time for more pressing concerns.

I hesitate to come back to this thread, because as Raymond says it's consumed a lot of time already. But I think this point is of broader interest than the specific few lines we've been discussing:

When small integer are disabled at compilation time (NSMALLPOSINTS=0 and a NSMALLNEGINTS=0), I suggest to remove code related to small integers. IMHO CHECK_SMALL_INT() macro is a good practical solution for that.

Victor, can you be more specific about the problem this is solving? I think the existing code in master really doesn't leave any problems in place that CHECK_SMALL_INT solves.

In master (as of 2702638), here's the code that CHECK_SMALL_INT would replace:

    if (IS_SMALL_INT(ival)) {
        return get_small_int((sdigit)ival);
    }

When NSMALLPOSINTS=0 and NSMALLNEGINTS=0 , this expands in the preprocessor to the equivalent of:

    if (0) {
        return (Py_UNREACHABLE(), NULL);
    }

(Specifically, it expands to whatever that expands to, since Py_UNREACHABLE and possibly NULL are also macros.)

A compiler that's attempting to do any significant optimization at all has to be able to discard code that's inside if (0); dead-code elimination is a necessary primitive for lots of other optimizations.

(And at a quick empirical sanity-check: gcc, clang, and msvc all do so at any optimization setting other than "disabled". In fact gcc and clang do so even with optimizations disabled.)

You made the case very nicely above that macros are evil. The IS_SMALL_INT macro is fairly mild, and it has a performance benefit on some platforms to justify it. But CHECK_SMALL_INT causes a return from the enclosing function, which seems quite high on the "evil macro" scale.

I'm unassigning this. It has been blown profoundly out of proportion. The recommendation of the senior most developer is being ignored, and now two developers are speaking in terms of strong belief systems and labeling long-stable code as "evil". This doesn't bode well and it makes it difficult to conduct reasoned discourse.

(Meanwhile, we have another have another long-term active developer who routinely adds new macros to existing code because he thinks that is an improvement. That portends a pointless tug-of-war predicated almost solely in not liking how other people code).

Recent commits for longobject.c

Revision: 5e63ab05f114987478a21612d918a1c0276fe9d2
Author: Greg Price <[email protected]>
Date: 19-8-25 1:19:37
Message:
bpo-37812: Convert CHECK_SMALL_INT macro to a function so the return is explicit. (GH-15216)

The concern for this issue is: implicit return from macro.
We can add a comment before the call sites of CHECK_SMALL_INT macro, to explain that there is a possible return.

Revision: 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2
Author: animalize <[email protected]>
Date: 19-9-6 14:00:56
Message:
replace inline function `is_small_int` with a macro version (GH-15710)

Then this commit is not necessary.

Revision: c6734ee7c55add5fdc2c821729ed5f67e237a096
Author: Sergey Fedoseev <[email protected]>
Date: 19-9-12 22:41:14
Message:
bpo-37802: Slightly improve perfomance of PyLong_FromUnsigned*() (GH-15192)

This commit introduced a compiler warning due to this line [1]:
d:\dev\cpython\objects\longobject.c(412): warning C4244: “function”: from “unsigned long” to “sdigit ”，may lose data

[1] the line:
return get_small_int((ival)); \
https://github.com/python/cpython/blob/master/Objects/longobject.c#L386

Revision: 42acb7b8d29d078bc97b0cfd7c4911b2266b26b9
Author: HongWeipeng <[email protected]>
Date: 19-9-18 23:10:15
Message:
bpo-35696: Simplify long_compare() (GH-16146)

IMO this commit reduces readability a bit.

We can sort out these problems.

Sorry, but there was nothing wrong with the CHECK_SMALL_INT macro, to my eyes, to begin with - except that it was burdened with an over-elaborate "do ... while(0)" wrapper.

Not all macros are _intended_ to be "cheap functions". Like this one, some are intended to be merely textual replacement, to minimize tedious and error-prone redundant typing, in a specific file. Even "bulletproofing them" with tricks like "do ... while(0)" cruft is counterproductive in such cases. They're not _intended_ to capture abstractions, neither for general use. This one was intended to check its argument and possibly return, exactly what its expansion did (after mentally tossing out the distracting "do while" noise).

Nobody competent to address logic errors in longobject.c was confused about that in the slightest. Knowing the details of how small integers may be cached is part of what "competent" means in this context, and what the macro does is so simple it's no burden to learn or to use.

Is there an "aesthetic code cleanup" patch that's ever turned out well? ;-)

labeling long-stable code as "evil".

Let me apologize about this bit -- I was thinking of the term in quotes (referring to the earlier comment), but I didn't write it clearly that way. I don't think any of this code is evil, past or present, and I regret that I carelessly gave that impression.

I think there isn't currently any problem to be solved here, and I hope we can all put our energy elsewhere. (For my part, I just spent the balance of the evening implementing an old feature request on unicodedata.)

Victor, I'd still be quite interested in better understanding your thinking (as this example relates to macros in general) if you'd like to reply, though this thread may not be the right place to continue. You can find my email in Git, and I'm on Zulip and Discourse; and I'd be happy to start or follow a thread in a forum you think appropriate. Or if you'd rather drop it entirely, that's fine too.

You can find my email in Git, and I'm on Zulip and Discourse; and I'd be happy to start or follow a thread in a forum you think appropriate. Or if you'd rather drop it entirely, that's fine too.

I think opening a thread in https://discuss.python.org/c/users to talk about deciding between the usage of functions and macros (discussing when each may be appropriate) would be beneficial to the community at large.

The recommendation of the senior most developer is being ignored, and now two developers are speaking in terms of strong belief systems and labeling long-stable code as "evil". This doesn't bode well and it makes it difficult to conduct reasoned discourse.

Apologies if I added to that, I certainly respect your judgement on this issue. Pretty much everyone involved in this discussion has more experience in working with the CPython C-API than I do, you most of all (as far I'm aware). My perspective was coming from someone attempting to understand it better, and explaining how those learning the C-API might find it confusing.

I don't find the code itself to be at all "evil", just potentially confusing. That long-stable code has provided a lot of benefit over the years, and I can definitely appreciate the effort that was put into writing it.

Is there an "aesthetic code cleanup" patch that's ever turned out well? ;-)

From what I can tell, any remotely extensive aesthetic code patch I've seen has been highly controversial. I think they can have value in some cases, but the value relative to the potential cost may have been a bit overstated in this particular case.

Navigation Menu

Repository navigation

Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT) #81993

14 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Make implicit returns explicit in longobject.c (in CHECK_SMALL_INT) #81993

Description

Activity

gnprice commented on Aug 10, 2019

gnprice commented on Aug 10, 2019

rhettinger commented on Aug 11, 2019

rhettinger commented on Aug 11, 2019

gnprice commented on Aug 11, 2019

rhettinger commented on Aug 11, 2019

gnprice commented on Aug 11, 2019

animalize commented on Aug 12, 2019

rhettinger commented on Aug 12, 2019

animalize commented on Aug 12, 2019

rhettinger commented on Aug 12, 2019

gnprice commented on Aug 24, 2019

14 remaining items

animalize commented on Sep 18, 2019

gnprice commented on Sep 18, 2019

rhettinger commented on Sep 18, 2019

vstinner commented on Sep 18, 2019

vstinner commented on Sep 19, 2019

aeros commented on Sep 20, 2019

gnprice commented on Sep 20, 2019

rhettinger commented on Sep 20, 2019

animalize commented on Sep 20, 2019

tim-one commented on Sep 20, 2019

gnprice commented on Sep 20, 2019

aeros commented on Sep 20, 2019

aeros commented on Sep 22, 2019

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions