GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36710: Add 'ceval' local variable to ceval.c by vstinner · Pull Request #12934 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Apr 24, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Add "struct _ceval_runtime_state *ceval = &_PyRuntime.ceval;" local
variables to function to better highlight the dependency on the
global variable _PyRuntime and to point directly to _PyRuntime.ceval
field rather than on the larger _PyRuntime.

Changes:

  • Add _PyRuntimeState_GetThreadState()
  • _PyThreadState_GET() now calls _PyRuntimeState_GetThreadState()
  • Add an explicit 'ceval' parameter to macros like
    COMPUTE_EVAL_BREAKER() or SIGNAL_PENDING_SIGNALS().
  • Add 'tstate' parameter to call_function(), do_call_core() and
    do_raise().
  • Add 'runtime' parameter to _Py_CURRENTLY_FINALIZING().
  • Declare 'runtime', 'tstate', 'ceval' and 'eval_breaker' variables
    as constant.

https://bugs.python.org/issue36710

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
vstinner requested a review from ericsnowcurrently April 24, 2019 14:02
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 24, 2019
@bedevere-bot bedevere-bot added the awaiting core review label Apr 24, 2019
@vstinner
vstinner requested review from methane and serhiy-storchaka April 24, 2019 14:02
@vstinner vstinner added the skip news label Apr 24, 2019
@vstinner

vstinner commented Apr 24, 2019

Copy link
Copy Markdown
Member Author

This change adds a "tstate" parameter to call_function(), do_call_core() and do_raise(): @jdemeyer proposed a similar change in PR #12839. Here my intent is not optimization, but more correctness: ensure that runtime, interp and tstate are consistent in ceval.c.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Apr 24, 2019
bpo-36616: optimize handling of thread state in function call code #12839
Closed
@vstinner

vstinner commented Apr 24, 2019

Copy link
Copy Markdown
Member Author

@jdemeyer: Would you mind to review my change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Apr 25, 2019
bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly #12667
Merged
@jdemeyer

jdemeyer commented Apr 26, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@jdemeyer: Would you mind to review my change?

As discussed on bpo-36710 (let's keep that discussion there), I'm missing the why: which problem does this PR solve? Some of these changes do make sense independently to clean things up (for example, reducing the number of _PyThreadState_GET() calls like #12839). But some of the changes also make things more complicated, so they need to be justified.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 2, 2019

Copy link
Copy Markdown
Member Author

cc @ericsnowcurrently: I was talking about this change with you.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented May 10, 2019

Copy link
Copy Markdown
Member Author

I rebased my PR on top of the master branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
bpo-36710: Add 'ceval' local variable to ceval.c
b2f4988 
Add "struct _ceval_runtime_state *ceval = &_PyRuntime.ceval;" local
variables to function to better highlight the dependency on the
global variable _PyRuntime and to point directly to _PyRuntime.ceval
field rather than on the larger _PyRuntime.

Changes:

* Add _PyRuntimeState_GetThreadState(runtime) macro.
* Add _PyEval_AddPendingCall(ceval, ...) and
  _PyThreadState_Swap(gilstate, ...) functions.
* _PyThreadState_GET() macro now calls
  _PyRuntimeState_GetThreadState() using &_PyRuntime.
* Add 'ceval' parameter to COMPUTE_EVAL_BREAKER(),
  SIGNAL_PENDING_SIGNALS(), _PyEval_SignalAsyncExc(),
  _PyEval_SignalReceived() and _PyEval_FiniThreads() macros and
  functions.
* Add 'tstate' parameter to call_function(), do_call_core() and
  do_raise().
* Add 'runtime' parameter to _Py_CURRENTLY_FINALIZING(),
  _Py_FinishPendingCalls() and _PyThreadState_DeleteExcept()
  macros and functions.
* Declare 'runtime', 'tstate', 'ceval' and 'eval_breaker' variables
  as constant.
@vstinner
vstinner merged commit 09532fe into python:master May 10, 2019
@bedevere-bot bedevere-bot removed the awaiting core review label May 10, 2019
@vstinner
vstinner deleted the ceval_runtime branch May 10, 2019 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently
@methane methane Awaiting requested review from methane
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vstinner @jdemeyer @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.