[NOGIL] thread local promotion state #26368

ngoldbaum · 2024-05-01T21:58:01Z

This makes the promotion state a thread local variable and mediates access to it via getter and setter functions.

In order to use it I extended the existing NPY_TLS macro to also check for some C11/C23 possibilities. This macro is public - is it problematic to update it? A github search indicates it's not used much. I'm not sure if the macro works on all supported platforms, let's see. I don't think there are any supported platforms or compilers that are not covered by one of the spellings for thread-local storage I added to the meson config but please let me know if you're aware of any issues.

I decided to mark this variable as thread-local irrespective of the GIL setting. I don't think marking these variables as thread-local adds any overhead. Are there any benchmarks that might be sensitive to overhead in setting or getting the promotion state?

The test I added reliably fails before the promotion state in the nogil build on main.

ngoldbaum · 2024-05-01T22:01:41Z

Looks like there are some linux issues that need smoothing out at least

seberg · 2024-05-02T12:13:08Z

I don't mind doing this, it certainly is better/correct also in the non no-gil branch. But, do you have an actual reason/failure you need this for?

I.e. I would like to just delete all of this, and the main reason I haven't already started with that, is that I think as long as backports to 2.0 are very likely, it might make things complicated.

ngoldbaum · 2024-05-02T12:50:33Z

The only way I could consistently trigger the data race was with a test like one I added. I understand the desire to ease backports and if you don't want this that's fine. I worry a little bit about occasional breakage in threaded nogil tests because there are a few spots in the code where the promotion state is used like a context variable. That said, those code paths have a data race even with the GIL I think, so maybe not a big deal?

If we don't upstream this I'd like to figure out the linux build issues, see if there are any other build issues, and upstream the NPY_TLS changes at least. I suspect there's other global state that will need to be made thread-local elsewhere in numpy. I don't care much if we decide to do it in NPY_TLS or in a private macro.

seberg · 2024-05-02T13:10:02Z

Well, I realize that this is a bug fix effectively. To be clear: I don't mind adding this on the grounds of backports!

What I mostly wanted to say is that np._set_promotion_state() shouldn't exist in 2.1 and even in 2.0 it shouldn't matter except for testing. The only places where np._set_promotion_state() actually changes away from the global default should be:

The NumPy test suite itself (i.e. you could even delete those tests if you don't want to backport this)!
Downstream users who are testing, i.e. not production code!

So, I don't mind reviewing+merging this at all and it seems that build issue is useful. But it seems to me like this fix, while correct, might not be worth the trouble.

ngoldbaum · 2024-05-02T13:31:17Z

The new test is really only using those functions to simulate the data race that happens when the promotion state gets juggled internally. Happy to remove all of this when we delete the promotion state code.

seberg · 2024-05-02T13:36:01Z

OK, will be nice and eventually necessary to get the NPY_TLS thing right, so might as well use this to figure it out, even if it is probably very temporary.

ngoldbaum · 2024-05-06T21:53:03Z

Almost working, but it looks like some of the windows test failures are real so I need to do some debugging on a windows box to figure out why __declspec(thread) isn't sufficient.

ngoldbaum · 2024-05-07T20:43:38Z

I think I've resolved all the build issues, although I'd like to run the tests with the spin issue fixed to see if there are problems with clang-cl.

seberg

Assuming this is almost ready, so had a closer look. LGTM, modulo the 2-3 simple comments/fixes. So will approve, but those need fixing.

I don't know if all the thread-local definitions are right, but they look good.

seberg · 2024-05-08T16:57:55Z

numpy/_core/src/multiarray/arraytypes.c.src

-                    npy_promotion_state == NPY_USE_WEAK_PROMOTION_AND_WARN
+        int promotion_state = get_npy_promotion_state();
+        if (promotion_state == NPY_USE_LEGACY_PROMOTION || (
+                    get_npy_promotion_state() == NPY_USE_WEAK_PROMOTION_AND_WARN


Suggested change

get_npy_promotion_state() == NPY_USE_WEAK_PROMOTION_AND_WARN

promotion_state == NPY_USE_WEAK_PROMOTION_AND_WARN

seberg · 2024-05-08T17:00:08Z

numpy/_core/src/multiarray/convert_datatype.h

@@ -12,7 +12,8 @@ extern NPY_NO_EXPORT npy_intp REQUIRED_STR_LEN[];
 #define NPY_USE_LEGACY_PROMOTION 0
 #define NPY_USE_WEAK_PROMOTION 1
 #define NPY_USE_WEAK_PROMOTION_AND_WARN 2
-extern NPY_NO_EXPORT int npy_promotion_state;
+
+extern NPY_NO_EXPORT NPY_TLS int npy_promotion_state;


Suggested change

extern NPY_NO_EXPORT NPY_TLS int npy_promotion_state;

Since you add the functions, keeping the definition strange?

Unless you go back to using it directly (if that is even technically possible).

seberg · 2024-05-08T17:00:44Z

numpy/_core/src/multiarray/convert_datatype.c

 NPY_NO_EXPORT PyObject *NO_NEP50_WARNING_CTX = NULL;
 NPY_NO_EXPORT PyObject *npy_DTypePromotionError = NULL;
 NPY_NO_EXPORT PyObject *npy_UFuncNoLoopError = NULL;

+NPY_NO_EXPORT NPY_TLS int npy_promotion_state = NPY_USE_LEGACY_PROMOTION;


Suggested change

NPY_NO_EXPORT NPY_TLS int npy_promotion_state = NPY_USE_LEGACY_PROMOTION;

static NPY_TLS int npy_promotion_state = NPY_USE_LEGACY_PROMOTION;

Make it static now that it you have the function accessors.

Thanks for pointing this out, it didn't occur to me but of course this is nicer.

seberg · 2024-05-08T17:01:05Z

numpy/_core/meson.build

@@ -262,7 +264,7 @@ foreach optional_attr: optional_variable_attributes
      return 0;
    }
  '''
-  if cc.compiles(code)
+  if cc.compiles(code, name: optional_attr[0])


Full discloser, I don't know why this is needed, but it seems fine :)

It adds output for the numpy build stdout when it checks for these:

Checking if "thread_local" compiles: NO Checking if "_Thread_local" compiles: YES Checking if "__thread" compiles: YES Checking if "__declspec(thread)" compiles: NO Checking for function "fallocate" : NO

I found it useful for debugging on CI.

nice solution.

ngoldbaum added the 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) label May 1, 2024

ngoldbaum mentioned this pull request May 1, 2024

ENH: Support free-threaded python build (tracking issue) #26157

Closed

16 tasks

ngoldbaum force-pushed the nogil-thread-local-promotion-state branch 2 times, most recently from 035c9bc to 04f941b Compare May 6, 2024 21:13

ngoldbaum force-pushed the nogil-thread-local-promotion-state branch 4 times, most recently from c653580 to 0cc359d Compare May 7, 2024 20:03

ngoldbaum added 6 commits May 8, 2024 10:56

ENH: make the promotion state use thread-local storage

9ef44a4

TST: add test for promotion state thread safety

688af9d

MNT: appease linter

8d0a8ad

TST: skip test using threads on WASM

b8f31dc

BUG: fix typo in __declspec(thread) check

bdc63a0

MNT: fix last remaining direct use of npy_promotion_state

2e82f75

ngoldbaum force-pushed the nogil-thread-local-promotion-state branch from 0cc359d to 2e82f75 Compare May 8, 2024 16:56

seberg approved these changes May 8, 2024

View reviewed changes

MNT: apply code review comments

18a3404

ngoldbaum merged commit 2a9b913 into numpy:main May 9, 2024

	get_npy_promotion_state() == NPY_USE_WEAK_PROMOTION_AND_WARN
	promotion_state == NPY_USE_WEAK_PROMOTION_AND_WARN

	NPY_NO_EXPORT NPY_TLS int npy_promotion_state = NPY_USE_LEGACY_PROMOTION;
	static NPY_TLS int npy_promotion_state = NPY_USE_LEGACY_PROMOTION;

Uh oh!

[NOGIL] thread local promotion state #26368

[NOGIL] thread local promotion state #26368

Uh oh!

Conversation

ngoldbaum commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented May 1, 2024

Uh oh!

seberg commented May 2, 2024

Uh oh!

ngoldbaum commented May 2, 2024

Uh oh!

seberg commented May 2, 2024

Uh oh!

ngoldbaum commented May 2, 2024

Uh oh!

seberg commented May 2, 2024

Uh oh!

ngoldbaum commented May 6, 2024

Uh oh!

ngoldbaum commented May 7, 2024

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

seberg May 8, 2024

Choose a reason for hiding this comment

Uh oh!

seberg May 8, 2024

Choose a reason for hiding this comment

Uh oh!

seberg May 8, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum May 8, 2024

Choose a reason for hiding this comment

Uh oh!

seberg May 8, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Sharukh-Repliq May 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngoldbaum commented May 1, 2024 •

edited

Loading