bpo-41531: Fix compilation of dict literals with more than 0xFFFF elements

[pablogsal]

https://bugs.python.org/issue41531

[pablogsal]

I just made the minimum edit to fix the problem but I would just remove this checkpoint and simply keep incrementing elements and leave the last check fill the dict, which is simpler and less error prone. Any reason we want to flush when we are in USHRT_MAX elements @markshannon ?

To be clear, this is what I propose:

diff --git a/Python/compile.c b/Python/compile.c
index b2beef9327..a348b13676 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -3893,19 +3893,7 @@ compiler_dict(struct compiler *c, expr_ty e)
             ADDOP_I(c, DICT_UPDATE, 1);
         }
         else {
-            if (elements == USHRT_MAX) {
-                if (!compiler_subdict(c, e, i - elements, i + 1)) {
-                    return 0;
-                }
-                if (have_dict) {
-                    ADDOP_I(c, DICT_UPDATE, 1);
-                }
-                have_dict = 1;
-                elements = 0;
-            }
-            else {
                 elements++;
-            }
         }
     }
     if (elements) {

[markshannon]

My guess is that the 0xFFFF limit is a hangover from when bytecode instruction operands where two bytes.
Getting rid of that limit makes sense. However, it does make sense to have some smaller limit, as building the dict requires 2*n stack slots in the frame.

I'm merging this as it fixes the issue and adds a test.
However, I'd like to clean up the code before the release.

[miss-islington]

Thanks @pablogsal for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-21853 is a backport of this pull request to the 3.9 branch.

[hroncok]

@pablogsal Thanks for the fix. I like the test :)

[pablogsal]

My guess is that the 0xFFFF limit is a hangover from when bytecode instruction operands where two bytes.
Getting rid of that limit makes sense. However, it does make sense to have some smaller limit, as building the dict requires 2*n stack slots in the frame.

I'm merging this as it fixes the issue and adds a test.
However, I'd like to clean up the code before the release.

Seems that this logic was introduced in this commit:

commit fd7ed407d79b797e20d0a6fe69e18f9ba9354979
Author: Raymond Hettinger <[email protected]>
Date:   Tue Dec 18 21:24:09 2007 +0000

    Give meaning to the oparg for BUILD_MAP:  estimated size of the dictionary.
    
    Allows dictionaries to be pre-sized (upto 255 elements) saving time lost
    to re-sizes with their attendant mallocs and re-insertions.
    
    Has zero effect on small dictionaries (5 elements or fewer), a slight
    benefit for dicts upto 22 elements (because they had to resize once
    anyway), and more benefit for dicts upto 255 elements (saving multiple
    resizes during the build-up and reducing the number of collisions on
    the first insertions).  Beyond 255 elements, there is no addional benefit.

diff --git a/Python/compile.c b/Python/compile.c
index 3b0c53fb19..36ad8a48e0 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -2922,11 +2922,9 @@ compiler_visit_expr(struct compiler *c, expr_ty e)
        case IfExp_kind:
                return compiler_ifexp(c, e);
        case Dict_kind:
-               /* XXX get rid of arg? */
-               ADDOP_I(c, BUILD_MAP, 0);
                n = asdl_seq_LEN(e->v.Dict.values);
-               /* We must arrange things just right for STORE_SUBSCR.
-                  It wants the stack to look like (value) (dict) (key) */
+               ADDOP_I(c, BUILD_MAP, (n>255 ? 255 : n));
+               n = asdl_seq_LEN(e->v.Dict.values);
                for (i = 0; i < n; i++) {
                        VISIT(c, expr, 
                                (expr_ty)asdl_seq_GET(e->v.Dict.values, i));

[miss-islington]

Thanks @pablogsal for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-22105 is a backport of this pull request to the 3.9 branch.

[miss-islington]

Thanks @pablogsal for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry @pablogsal and @markshannon, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker c51db0ea40ddabaf5f771ea633b37fcf4c90a495 3.9

[miss-islington]

Thanks @pablogsal for the PR, and @markshannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[bedevere-bot]

GH-22107 is a backport of this pull request to the 3.9 branch.