Skip to content
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.

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

gh-112535: Add test on _Py_ThreadId() #112709

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 4, 2023

Add also test.support.Py_GIL_DISABLED constant.

@vstinner
Copy link
Member Author

vstinner commented Dec 4, 2023

@colesbury @corona10: I wrote a short test to spawn 5 threads, call _Py_ThreadId() in the main thread and in the 5 threads, and then make sure that we get 6 unique numbers. What do you think of this test? Is it relevant?

Maybe we can compare _Py_ThreadId() with _thread.get_ident() or _thread.get_native_id()? I added a test to compare with _thread.get_ident() on Linux x86-64.

@vstinner
Copy link
Member Author

vstinner commented Dec 4, 2023

On FreeBSD x86-64, _Py_ThreadId() cannot be compared to Thread.ident or Thread.native_id:

vstinner@freebsd$ ./python
Python 3.13.0a2+ (heads/pr/112709:bf6b57b95b, Dec  4 2023, 15:05:07) [Clang 16.0.6 (https://github.com/llvm/llvm-project.git llvmorg-16.0.6-0-g7cbf1a on freebsd14

>>> import _testinternalcapi; hex(_testinternalcapi.py_thread_id()) 
'0x32c2e5025140'
>>> import _thread; hex(_thread.get_ident())
'0x32c2e5412000'
>>> _thread.get_native_id()
100125

@vstinner
Copy link
Member Author

vstinner commented Dec 4, 2023

On FreeBSD x86-64, _Py_ThreadId() cannot be compared to Thread.ident or Thread.native_id:

Same on Linux ppc64le.

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.

I think having a test for _PyThread_Id() is good, but I I don't think we should bother comparing with _thread.get_ident().

I think we want the following:

  • _PyThread_Id() should be unique and non-zero
  • _PyThread_Id() should not change for a given thread. For example, it should remain the same after a short sleep.
  • It should be a multiple of four (i.e., at least 4-byte aligned). This isn't necessary for biased reference counting, but it makes implementing recursive mutexes easier and should be true for all our implementations of it.

get_py_thread_id(PyObject *self, PyObject *type)
{
uintptr_t tid = _Py_ThreadId();
Py_BUILD_ASSERT(sizeof(unsigned long long) >= sizeof(tid));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use C11 static_assert() now, which has better error messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Py_BUILD_ASSERT() doesn't require an error message which makes my life easier, I don't have to write down an error message :-)

Add also test.support.Py_GIL_DISABLED constant.
@vstinner
Copy link
Member Author

vstinner commented Dec 4, 2023

I think having a test for _PyThread_Id() is good, but I I don't think we should bother comparing with _thread.get_ident().

Ok, I removed this test.

_PyThread_Id() should be unique and non-zero
_PyThread_Id() should not change for a given thread. For example, it should remain the same after a short sleep.

I added tests for that.

It should be a multiple of four (i.e., at least 4-byte aligned). This isn't necessary for biased reference counting, but it makes implementing recursive mutexes easier and should be true for all our implementations of it.

I will let you add a check for that later (in a following PR). I prefer to add tests step by step, especially if it's not strictly needed right now.

@vstinner
Copy link
Member Author

vstinner commented Dec 4, 2023

@colesbury: Would you mind to review the updated PR?

@@ -112,7 +112,7 @@ def test_module_not_found(self):
class WindowsExtensionSuffixTests:
def test_tagged_suffix(self):
suffixes = self.machinery.EXTENSION_SUFFIXES
abi_flags = "t" if sysconfig.get_config_var("Py_GIL_DISABLED") else ""
abi_flags = "t" if support.Py_GIL_DISABLED else ""
Copy link
Contributor

@colesbury colesbury Dec 4, 2023

Choose a reason for hiding this comment

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

@vstinner - looks like Windows tests are failing (possibly due to missing import for support:

https://github.com/python/cpython/actions/runs/7090430380/job/19297369118?pr=112709

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, I saw it before. Thanks for the reminder. It should now be fixed.

@brettcannon brettcannon removed their request for review December 4, 2023 20:13
@vstinner vstinner enabled auto-merge (squash) December 4, 2023 22:39
@vstinner vstinner merged commit c5fa8a5 into python:main Dec 4, 2023
30 checks passed
@vstinner vstinner deleted the test_py_threadid branch December 4, 2023 22:40
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Add also test.support.Py_GIL_DISABLED constant.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Add also test.support.Py_GIL_DISABLED constant.
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.

3 participants