Opened 9 months ago

Closed 9 months ago

Last modified 8 months ago

#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) 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 9 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-27256
  • Commit set to 3aa09789a306ab0133b69face0d5e8ec16e3e959
  • Status changed from new to needs_review

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:

3aa0978Trac #27256: Explicitly build OpenBLAS with the USE_TLS=1 flag

comment:2 follow-up: Changed 9 months ago by 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.

comment:3 in reply to: ↑ 2 Changed 9 months ago by embray

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 9 months ago by fbissey

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 9 months ago by embray

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 9 months ago by fbissey

  • 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 9 months ago by vbraun

  • Branch changed from u/embray/ticket-27256 to 3aa09789a306ab0133b69face0d5e8ec16e3e959
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:8 Changed 8 months ago by embray

  • Commit 3aa09789a306ab0133b69face0d5e8ec16e3e959 deleted

Follow-up in #27438 without which this is broken.

Note: See TracTickets for help on using tickets.