#27256 closed defect (fixed)
Re-enable USE_TLS=1 when building OpenBLAS SPKG
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.7 |
Component: | packages: standard | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | François Bissey |
Report Upstream: | N/A | Work issues: | |
Branch: | 3aa0978 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
OpenBLAS has a flag that can be passed at build time called USE_TLS
.
This enables or disables an "experimental" feature which makes some changes to memory allocation models for multi-threaded code, where previously threads had to share some global data structures related to memory allocation that were protected by locks. With USE_TLS=1
a different data structure is used such that thread-specific data is stored in thread-local storage, rather than global variables. This seems (unsurprisingly) to introduce some performance benefits in many cases, though the details are a bit vague.
In OpenBLAS v0.3.3 this feature was enabled by default, and to my knowledge did not cause any problems. However, in v0.3.5 it's now disabled by default.
I would suggest that we re-enable it, until and unless it is known to cause any problems. And incidentally, so-doing would fix #27213, which was caused by code paths that are only compiled if USE_TLS=0
. The alternative fix to #27213 would be to apply a patch, but I think it would be better to go ahead and re-enable this flag, since it purportedly has performance benefits, and no downside that we currently know of.
Change History (8)
comment:1 Changed 3 years ago by
- Branch set to u/embray/ticket-27256
- Commit set to 3aa09789a306ab0133b69face0d5e8ec16e3e959
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 3 years ago by
Hopefully they have really fixed that code path on all platforms. This was the stuff that was causing error about not being able to allocate more thread or something. There was a problem with how they were counting threads.
comment:3 in reply to: ↑ 2 Changed 3 years ago by
Replying to fbissey:
Hopefully they have really fixed that code path on all platforms. This was the stuff that was causing error about not being able to allocate more thread or something. There was a problem with how they were counting threads.
Could you be more specific? I'm not sure exactly what you're referring to or on what platform.
FWIW this is the PR to fix it. The problem affected a lot of things beyond just Cygwin, badly enough that I think there should be an 0.3.6 release, with 0.3.5 marked as verboten: https://github.com/xianyi/OpenBLAS/pull/2004/files
comment:4 Changed 3 years ago by
I'll just refer to the release notes for openblas-0.3.3
thread memory allocation has been switched back to the method used before version 0.3.1 due to unexpected problems caused by the new code under some circumstances. A new compile-time option USE_TLS has been added to allow enabling the new code instead, and it is hoped that this can become the default again in the next version.
There was a number of reports that forced this switch back and it is still not the default as of 0.3.5. However I am ok with it if you think it is safe and the bots are OK.
comment:5 Changed 3 years ago by
Well, all I can say is that in 0.3.3 USE_TLS=1
was the default, and was working fine as far as I know. Obviously if there is some specific report of a problem that can be attributed to it that's different, but I don't know of any.
comment:6 Changed 3 years ago by
- Reviewers set to François Bissey
- Status changed from needs_review to positive_review
Let's just go ahead with it. If there is a problem we'll know soon enough.
comment:7 Changed 3 years ago by
- Branch changed from u/embray/ticket-27256 to 3aa09789a306ab0133b69face0d5e8ec16e3e959
- Resolution set to fixed
- Status changed from positive_review to closed
comment:8 Changed 3 years ago by
- Commit 3aa09789a306ab0133b69face0d5e8ec16e3e959 deleted
Follow-up in #27438 without which this is broken.
Confirmed that this fixes #27213. It should have no (known) detriment on other platforms we support, since this option was the default on OpenBLAS 0.3.3.
New commits:
Trac #27256: Explicitly build OpenBLAS with the USE_TLS=1 flag