Skip to content

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jun 18, 2024

}
err:
Copy link
Member

Choose a reason for hiding this comment

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

To have a more regular try: ... finally: _PyEval_StartTheWorld() pattern, you can add an int res = -1; variable, replace goto err with goto done, and set res to 0 on success (3 lines above).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @vstinner's suggestion is fine. Or you can refactor the parts that should be in a stop-the-world call into it's own function, like we often do for locks.

Another advantage of moving the body to a separate function is that it makes it more clear what data crosses the stop-the-world boundary -- some data loaded before the stop-the-world call may not be valid after it.

@@ -323,6 +323,8 @@ _PyInlineValuesSize(PyTypeObject *tp)
int
_PyDict_DetachFromObject(PyDictObject *dict, PyObject *obj);

PyDictObject *_PyObject_materialize_managed_dict_lock_held(PyObject *);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name this like the other functions: _PyObject_MaterializeManagedDict_LockHeld and move the definition up next to _PyObject_MaterializeManagedDict.

}
err:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @vstinner's suggestion is fine. Or you can refactor the parts that should be in a stop-the-world call into it's own function, like we often do for locks.

Another advantage of moving the body to a separate function is that it makes it more clear what data crosses the stop-the-world boundary -- some data loaded before the stop-the-world call may not be valid after it.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments about comments.

I think this should be backported to 3.13

@Fidget-Spinner Fidget-Spinner merged commit 3bfc9c8 into python:main Jul 10, 2024
@miss-islington-app
Copy link

Thanks @Fidget-Spinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @Fidget-Spinner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3bfc9c831ad9a3dcf4457e842f1e612e93014a17 3.13

@Fidget-Spinner Fidget-Spinner deleted the class_stoptheworld branch July 10, 2024 18:02
Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Jul 10, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@hugovk hugovk removed the needs backport to 3.13 bugs and security fixes label Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants