Skip to content

bpo-37146: Allow to configure opcode cache and fix huntleaks by presetting it to 1#13789

Closed
pablogsal wants to merge 4 commits intopython:masterfrom
pablogsal:refleaks
Closed

bpo-37146: Allow to configure opcode cache and fix huntleaks by presetting it to 1#13789
pablogsal wants to merge 4 commits intopython:masterfrom
pablogsal:refleaks

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 3, 2019 •
edited by bedevere-bot
Loading

@pablogsal pablogsal requested review from methane and vstinner June 3, 2019 22:22
@pablogsal
Copy link
Member Author

pablogsal commented Jun 3, 2019

Tested locally:

❯ ./python -m test -R 3:3 test_pprint
Run tests sequentially
0:00:00 load avg: 1.57 [1/1] test_pprint
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 751 ms
Tests result: SUCCESS

@pablogsal
Copy link
Member Author

pablogsal commented Jun 3, 2019

Tested also with ./python -m test -R 3:3 -j0 -r -x test_asyncio -x test_multiprocessing_spawn

@pablogsal pablogsal changed the title Allow to configure minruns of the opcode cache and fix huntleaks by presetting it to 1 bpo-37146: Allow to configure opcode cache and fix huntleaks by presetting it to 1 Jun 3, 2019
vstinner
vstinner requested changes Jun 3, 2019
View reviewed changes
Include/internal/pycore_pystate.h Outdated
Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

the struct is useless, just put directly the int into _ceval_runtime_state.

Copy link
Member Author

@pablogsal pablogsal Jun 3, 2019

Choose a reason for hiding this comment

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

I made a struct for easy extending more stuff in the config in the future.

I will remove it

Include/internal/pycore_pystate.h Outdated
Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

you should use: (void)

Lib/test/libregrtest/runtest.py Outdated
Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

I don't think that it matters to restore the old value. I'm not sure that sys._getopcacheminruns() is needed.

Python/pylifecycle.c Outdated
Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

pleave remove this XXX. It doesn't have to be a cmdline option.

Lib/test/libregrtest/runtest.py Outdated
Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

That's wrong. Only dash_R() should be modified in libregrtest/refleak.py. You must not disable the opcache in runtest.py.

Copy link
Member Author

@pablogsal pablogsal Jun 3, 2019

Choose a reason for hiding this comment

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

Sorry, this was supposed to reset the original value. Check out my last fixup (bd3a951),

Copy link
Member

@vstinner vstinner Jun 3, 2019

Choose a reason for hiding this comment

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

I think you misunderstood my comment. You must not modify this function but dash_R() function.

@bedevere-bot
Copy link

bedevere-bot commented Jun 3, 2019

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

@methane
Copy link
Member

methane commented Jun 3, 2019

The code object will be optimized only when ++co->co_opcache_flag == opcacheminruns.
So decreasing min_runs by _setopcacheminruns()will cause some hot codes will be not
optimized forever. I don't want to expose such switch.

Please use #13787 for 3.8b1.

@vstinner
Copy link
Member

vstinner commented Jun 3, 2019

Please use #13787 for 3.8b1.

I like #13787, it's a reasonable workaround for beta1. We can design a better fix after the beta.

@pablogsal
Copy link
Member Author

pablogsal commented Jun 3, 2019

Ok, I will close this PR then.

@pablogsal pablogsal closed this Jun 3, 2019
@vstinner
Copy link
Member

vstinner commented Jun 3, 2019

Ok, I will close this PR then.

Maybe keep it around, it's sounds like a good fix for post-beta1.

@pablogsal pablogsal deleted the refleaks branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

@methane methane Awaiting requested review from methane

Assignees

No one assigned

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal @bedevere-bot @methane @vstinner @the-knights-who-say-ni