Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

pablogsal
Copy link
Member

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

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

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

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
@@ -43,6 +43,10 @@ struct _pending_calls {
int last;
};

struct _opcodecache_config {
int minruns;
};
Copy link
Member

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

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
@@ -312,6 +317,10 @@ PyAPI_FUNC(void) _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime);

PyAPI_FUNC(void) _PyGILState_Reinit(_PyRuntimeState *runtime);

PyAPI_FUNC(void) _PyEval_SetOpcodeCacheMinRuns(int minruns);

PyAPI_FUNC(int) _PyEval_GetOpcodeCacheMinRuns();
Copy link
Member

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
@@ -206,6 +206,8 @@ def _test_module(the_module):
def _runtest_inner2(ns, test_name):
# Load the test function, run the test function, handle huntrleaks
# and findleaks to detect leaks
minruns = sys._getopcacheminruns()
Copy link
Member

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
@@ -737,6 +737,9 @@ _Py_PreInitializeFromPyArgv(const PyPreConfig *src_config, const _PyArgv *args)
return status;
}

// XXX: Add this properly to the commandline config
Copy link
Member

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
@@ -206,6 +206,8 @@ def _test_module(the_module):
def _runtest_inner2(ns, test_name):
# Load the test function, run the test function, handle huntrleaks
# and findleaks to detect leaks
minruns = sys._getopcacheminruns()
sys._setopcacheminruns(1)
Copy link
Member

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

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

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

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

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