Skip to content

gh-99254: remove all unused consts from code objects#99255

Merged
iritkatriel merged 19 commits intopython:mainfrom
iritkatriel:remove_unused_consts
Nov 11, 2022
Merged

gh-99254: remove all unused consts from code objects#99255
iritkatriel merged 19 commits intopython:mainfrom
iritkatriel:remove_unused_consts

Conversation

@iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Nov 8, 2022 •
edited
Loading

This reduces the total size of unused consts in the top 100 PyPl packages by about 2%:

Before:

Total: 75 errors; 9,946 files; 235,684 code objects; 3,669,436 lines; 31,309,347 opcodes; 31,073,663 opcode pairs; 12,916,440.0 cache_size; 9,198,802.0 cache wasted; 1,858,819 ops quickened; 44,504 prev extended args; 1,509,350 total size of co_consts; 189,300 number of co_consts

After:

Total: 75 errors; 9,946 files; 235,684 code objects; 3,669,436 lines; 31,307,877 opcodes; 31,072,193 opcode pairs; 12,915,869.0 cache_size; 9,198,231.0 cache wasted; 1,858,819 ops quickened; 43,034 prev extended args; 1,477,889 total size of co_consts; 189,286 number of co_consts

@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 8, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 8, 2022
Closed
@iritkatriel iritkatriel requested a review from sweeneyde November 8, 2022 17:48
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022
@bedevere-bot
Copy link

bedevere-bot commented Nov 8, 2022

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit a9f38fd 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 8, 2022
kumaraditya303
kumaraditya303 reviewed Nov 9, 2022
View reviewed changes
iritkatriel and others added 2 commits November 9, 2022 18:45
@iritkatriel @kumaraditya303
use PyList_GET_SIZE
3c63ffe 
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@iritkatriel @kumaraditya303
use PyList_GET_SIZE
a470862 
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
thatbirdguythatuknownot
thatbirdguythatuknownot reviewed Nov 9, 2022
View reviewed changes
sweeneyde
sweeneyde approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. A couple of little things, but they could go either way.

@sweeneyde
Copy link
Member

sweeneyde commented Nov 10, 2022 •
edited
Loading

Unfortunately, I can reproduce the Windows failures locally.

I get a segfault when Py_DECREF(0x00000000ffffffff) happens in some POP_TOP instruction. Running .\python.bat -m test with lltrace=1 and commenting out dump_stack() gave output that ended with the following:

Details 
Resuming frame for 'Enum.__new__' in module 'enum'
0: RESUME 0
2: LOAD_GLOBAL_BUILTIN 1
14: LOAD_FAST 1
16: CALL_NO_KW_TYPE_1 1
26: LOAD_FAST 0
28: IS_OP 0
30: POP_JUMP_IF_FALSE 2
36: NOP
38: LOAD_FAST 0
40: LOAD_ATTR 2
40: LOAD_ATTR 2
60: LOAD_FAST 1
62: BINARY_SUBSCR_DICT
72: RETURN_VALUE

Resuming frame for 'EnumType.__call__' in module 'enum'
42: RETURN_VALUE

Resuming frame.2: INTERPRETER_EXIT
246: RETURN_VALUE

Resuming frame.2: INTERPRETER_EXIT
522: POP_JUMP_IF_FALSE 2
528: LOAD_GLOBAL_BUILTIN 39
540: LOAD_GLOBAL_MODULE 12
552: CALL_NO_KW_LEN 1
562: LOAD_GLOBAL_MODULE 40
574: COMPARE_OP_INT_JUMP 5
714: LOAD_FAST 3
716: LOAD_GLOBAL_MODULE 12
728: LOAD_FAST 2
730: STORE_SUBSCR_DICT
734: LOAD_GLOBAL_BUILTIN 39
746: LOAD_GLOBAL_MODULE 6
758: CALL_NO_KW_LEN 1
768: LOAD_GLOBAL_MODULE 50
780: COMPARE_OP_INT_JUMP 5
920: LOAD_FAST 3
922: LOAD_GLOBAL_MODULE 6
934: LOAD_FAST 2
936: STORE_SUBSCR_DICT
940: LOAD_FAST 3
942: RETURN_VALUE

Resuming frame for 'compile' in module 're'
28: RETURN_VALUE

Resuming frame for '<module>'
262: STORE_NAME 37
264: EXTENDED_ARG 5
266: LOAD_CONST 174
268: LOAD_CONST 24
270: MAKE_FUNCTION 1
272: STORE_NAME 38
274: EXTENDED_ARG 5
276: LOAD_CONST 174
278: LOAD_CONST 25
280: MAKE_FUNCTION 1
282: STORE_NAME 39
284: EXTENDED_ARG 5
286: LOAD_CONST 174
288: LOAD_CONST 26
290: MAKE_FUNCTION 1
292: STORE_NAME 40
294: EXTENDED_ARG 5
296: LOAD_CONST 175
298: LOAD_CONST 27
300: MAKE_FUNCTION 1
302: STORE_NAME 41
304: LOAD_CONST 28
306: MAKE_FUNCTION 0
308: STORE_NAME 7
310: LOAD_CONST 29
312: MAKE_FUNCTION 0
314: STORE_NAME 42
316: EXTENDED_ARG 5
318: LOAD_CONST 174
320: LOAD_CONST 30
322: MAKE_FUNCTION 1
324: STORE_NAME 43
326: LOAD_NAME 44
328: BUILD_TUPLE 1
330: LOAD_CONST 31
332: MAKE_FUNCTION 1
334: STORE_NAME 45
336: LOAD_CONST 32
338: MAKE_FUNCTION 0
340: STORE_NAME 46
342: LOAD_CONST 33
344: MAKE_FUNCTION 0
346: STORE_NAME 47
348: LOAD_NAME 26
350: STORE_NAME 48
352: LOAD_CONST 34
354: MAKE_FUNCTION 0
356: STORE_NAME 49
358: LOAD_CONST 35
360: MAKE_FUNCTION 0
362: STORE_NAME 50
364: LOAD_CONST 36
366: MAKE_FUNCTION 0
368: STORE_NAME 51
370: LOAD_CONST 37
372: MAKE_FUNCTION 0
374: STORE_NAME 52
376: LOAD_CONST 38
378: MAKE_FUNCTION 0
380: STORE_NAME 53
382: EXTENDED_ARG 5
384: LOAD_CONST 176
386: LOAD_CONST 39
388: MAKE_FUNCTION 1
390: STORE_NAME 54
392: LOAD_NAME 18
394: BUILD_TUPLE 1
396: LOAD_CONST 40
398: MAKE_FUNCTION 1
400: STORE_NAME 55
402: EXTENDED_ARG 5
404: LOAD_CONST 172
406: LOAD_CONST 41
408: MAKE_FUNCTION 1
410: STORE_NAME 26
412: LOAD_NAME 16
414: BUILD_TUPLE 1
416: LOAD_CONST 42
418: MAKE_FUNCTION 1
420: STORE_NAME 56
422: NOP
424: LOAD_CONST 1
426: LOAD_CONST 43
428: IMPORT_NAME 13
430: IMPORT_FROM 57
432: STORE_NAME 57
434: POP_TOP
436: NOP
438: LOAD_NAME 58
8846: POP_JUMP_IF_FALSE 9
8848: POP_TOP

Maybe the remove_unused_consts call needs to be moved earlier, to the /** Optimization **/ section, so that EXTENDED_ARGs won't have been added yet.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 10, 2022

Maybe the remove_unused_consts call needs to be moved earlier, to the /** Optimization **/ section, so that EXTENDED_ARGs won't have been added yet.

I think it is before - extended args are added in assemble_emit.

@brandtbucher
Copy link
Member

brandtbucher commented Nov 10, 2022 •
edited
Loading

I think it is before - extended args are added in assemble_emit.

But jump distances are computed in assemble_jump_offsets, which happens before this. If any EXTENDED_ARGs are removed from LOAD_CONST instructions, anything that jumps over them will be wrong.

brandtbucher
brandtbucher previously requested changes Nov 10, 2022
View reviewed changes
@bedevere-bot
Copy link

bedevere-bot commented Nov 10, 2022

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 10, 2022

I think it is before - extended args are added in assemble_emit.

But jump distances are computed in assemble_jump_offsets, which happens before this. If any EXTENDED_ARGs are removed from LOAD_CONST instructions, anything that jumps over them will be wrong.

Ah right, there's a comment about this in the code.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 10, 2022

Ok, that fixed it. I wonder why it only showed up on windows.

@brandtbucher
Copy link
Member

brandtbucher commented Nov 10, 2022

I wonder why it only showed up on windows.

I bet the startup sequence contains quite a bit of Windows-specific code. Probably just a really lucky/unlucky code path.

@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 10, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Nov 10, 2022

Thanks for making the requested changes!

@sweeneyde, @brandtbucher: please review the changes made to this pull request.

@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 11, 2022
@bedevere-bot
Copy link

bedevere-bot commented Nov 11, 2022

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 02b68e0 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 11, 2022
@iritkatriel iritkatriel dismissed brandtbucher’s stale review November 11, 2022 09:56

No need for re-review.

@iritkatriel iritkatriel merged commit 3dd6ee2 into python:main Nov 11, 2022
@iritkatriel
Copy link
Member Author

iritkatriel commented Nov 11, 2022

Thanks for the reviews and debugging help.

ethanfurman pushed a commit to ethanfurman/cpython that referenced this pull request Nov 12, 2022
@iritkatriel @ethanfurman
pythongh-99254: remove all unused consts from code objects (pythonGH-…
3192ce3 
…99255)
@iritkatriel iritkatriel deleted the remove_unused_consts branch December 7, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

@sweeneyde sweeneyde Awaiting requested review from sweeneyde

+1 more reviewer

@thatbirdguythatuknownot thatbirdguythatuknownot thatbirdguythatuknownot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@iritkatriel @bedevere-bot @sweeneyde @brandtbucher @kumaraditya303 @thatbirdguythatuknownot