Skip to content

bpo-37271: Optimize bytecode multiple times until it cannot be optimized further - #14068

Closed
pablogsal wants to merge 6 commits into
python:masterfrom
pablogsal:multiple_bytecode_opt
Closed

bpo-37271: Optimize bytecode multiple times until it cannot be optimized further#14068
pablogsal wants to merge 6 commits into
python:masterfrom
pablogsal:multiple_bytecode_opt

Conversation

@pablogsal

@pablogsalpablogsal commented Jun 14, 2019•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@nedbat

nedbat commented Jun 14, 2019

Copy link
Copy Markdown
Member

@pablogsal Any chance you want to help get #13600 finished?

@pablogsal

pablogsal commented Jun 14, 2019

Copy link
Copy Markdown
MemberAuthor

@nedbat Sure, I will talk with Victor to see if we can push it forward.

@pablogsalpablogsal changed the title Optimize bytecode multiple times until it cannot be optimized furtherbpo-37271: Optimize bytecode multiple times until it cannot be optimized furtherJun 14, 2019
@pablogsal
pablogsal marked this pull request as ready for review June 14, 2019 02:00
@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch 2 times, most recently from 576034a to f9b5588CompareJune 14, 2019 02:04
@pablogsalpablogsal self-assigned this Jun 14, 2019
@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch 2 times, most recently from 64fa0b9 to 8188a1aCompareJune 14, 2019 03:15
@pablogsal

pablogsal commented Jun 15, 2019

Copy link
Copy Markdown
MemberAuthor

@serhiy-storchaka What do you think about this approach?

@brettcannonbrettcannon added the performance Performance or resource usage label Jun 21, 2019
@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch from 8188a1a to 282da9eCompareJuly 13, 2019 15:06
ZackerySpytz
ZackerySpytz reviewed Jul 26, 2019
View reviewed changes
Comment threadPython/peephole.c Outdated
Comment threadPython/peephole.c Outdated
Comment threadPython/peephole.c Outdated
Comment threadPython/peephole.c Outdated
Comment threadPython/peephole.c Outdated
@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch from 9119526 to 68a2657CompareJuly 29, 2019 11:15
@pablogsal

pablogsal commented Jul 29, 2019

Copy link
Copy Markdown
MemberAuthor

I have rebased and address the feedback. Also, I have added a max cap of iterations (although is not needed, it can be proven that the while loop will always finish) to make sure the time expended in the loop is always bounded.

@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch 2 times, most recently from d9f5d4a to 99bb01aCompareJuly 29, 2019 11:17
@pablogsal
pablogsal requested a review from tim-oneJuly 29, 2019 11:17
@pablogsal
pablogsalforce-pushed the multiple_bytecode_opt branch 2 times, most recently from 7aed1c2 to a5ec1beCompareJuly 29, 2019 14:19
markshannon
markshannon reviewed Sep 9, 2019
View reviewed changes

@markshannonmarkshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good in general, just a couple of questions.

Comment threadPython/peephole.c Outdated
Comment threadPython/peephole.c
@pablogsal

pablogsal commented Sep 9, 2019

Copy link
Copy Markdown
MemberAuthor

@markshannon I have added some comments as requested, check if they look good to you :)

@pablogsal

pablogsal commented Sep 10, 2019

Copy link
Copy Markdown
MemberAuthor

CC: @Yhg1s

@pablogsal

pablogsal commented Sep 10, 2019

Copy link
Copy Markdown
MemberAuthor

This is the code that is failing the assert currently:

def f(cond1, cond2):
while 1:
return 3
while 1:
return 5
return 6

@Yhg1s

Yhg1s commented Sep 11, 2019

Copy link
Copy Markdown
Member

GH-15970 should fix the lnotab issue.

@pablogsalpablogsal mentioned this pull request Sep 11, 2019
Closed
@pablogsal
pablogsal deleted the multiple_bytecode_opt branch May 19, 2021 18:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannonmarkshannonmarkshannon left review comments
@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka
@tim-onetim-oneAwaiting requested review from tim-one
+1 more reviewer
@ZackerySpytzZackerySpytzZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@pablogsalpablogsal

Labels

awaiting core reviewperformancePerformance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@pablogsal@nedbat@Yhg1s@markshannon@ZackerySpytz@brettcannon@the-knights-who-say-ni@bedevere-bot