-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Avoid calling wcsset if the wcsprm is unchanged #16411
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
Just to put it on the table, an alternative is to compute a checksum of the wcsprm, but that would be more work as there are a lot of attributes on wcsprm. |
astropy/wcs/src/wcslib_wrap.c
Outdated
// using x_prev, and we compare these. If equal, and if PyWcsprm_cset() succeeded | ||
// last time it was called, then we can just return 0 as there should be nothing | ||
// to do. | ||
if(self->x_prev_set) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self->x_prev_set
might need to be initialised somewhere in the init (perhaps where x.flag
is initialised) otherwise this could potentially be a use of uninitialised memory
astropy/wcs/src/wcslib_wrap.c
Outdated
wcsprm_c2python(&self->x); | ||
wcsprm_c2python(&self->x_prev); | ||
if (status == 0 && equal != 0) return 0; | ||
} | ||
|
||
if (convert) wcsprm_python2c(&self->x); | ||
status = wcsset(&self->x); | ||
if (convert) wcsprm_c2python(&self->x); | ||
|
||
if (status == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issue: From what I can tell, wcsset
is called and returns successfully even when given incomplete details, i.e., with naxis = 0
. May be we can add the status == 0 && naxis > 0
for only storing a fully initialised wcsprm
struct.
[Adding what Tom and I discussed over slack] This fix is a trade-off in ensuring that (The correct solution would require modifying all relevant getters (where the list of relevant getters might change in the future) to store the present values within a malloc'ed buffer, the number of data bytes, and another address that contains the memory address the getter refers to. Any call to |
Here are the relevant fields that require a * - wcsprm::naxis (q.v., not normally set by the user),
* - wcsprm::crpix,
* - wcsprm::pc,
* - wcsprm::cdelt,
* - wcsprm::crval,
* - wcsprm::cunit,
* - wcsprm::ctype,
* - wcsprm::lonpole,
* - wcsprm::latpole,
* - wcsprm::restfrq,
* - wcsprm::restwav,
* - wcsprm::npv,
* - wcsprm::pv,
* - wcsprm::nps,
* - wcsprm::ps,
* - wcsprm::cd,
* - wcsprm::crota,
* - wcsprm::altlin,
* - wcsprm::ntab,
* - wcsprm::nwtb,
* - wcsprm::tab,
* - wcsprm::wtb. |
With wcslib v8.3, there is a new function |
50a15cc
to
fc69c4a
Compare
@manodeep - I've now rebased on top of the WCSLIB 8.3 PR, and have then made it so we check the checksum prior to calling wcsset. The last commit is, I think a necessary bug fix in WCSLIB, which I've reported to Mark. With this PR, I don't see the multi-threading issues anymore! (well the ones related to wcsset) |
24d7293
to
2a18fcc
Compare
The WCSLIB bug fixes were approved by Mark - he said he will not release 8.3.1 yet but we can always hold off from merging this until it is if people prefer not to patch WCSLIB ourselves in the meantime. @manodeep - in the meantime, could you review this and let me know what you think? |
This is not easy to properly test in our test suite - it would need to rely on a multi-threading test and the behavior is non-deterministic so I'm not sure how useful it would be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made two minor comments - looks good to me
@@ -1622,6 +1622,10 @@ PyWcsprm_cset( | |||
|
|||
int status = 0; | |||
|
|||
// We want to avoid calling wcsset whenever possible as it is not thread-safe. We use wcsenq | |||
// to see if the checksum of the wcsprm elements has changed since wcsset was last called. | |||
if (wcsenq(&self->x, WCSENQ_CHK)) return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worthwhile to add a comment that wcsenq
itself is also not thread-safe, and therefore, should only be invoked while holding the GIL (i.e., not within a threaded region)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, which part of wcsenq is not thread-safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was totally wrong! I thought chksum
was the variable in the wcsprm
struct but it is not. chksum
is a function local variable and therefore, all threads should compute the same value of chksum in wcsenq
docs/changes/wcs/16411.bug.rst
Outdated
@@ -0,0 +1 @@ | |||
Fixed an issue which caused calls to WCS coordinate conversion routines to not be thread-safe due to repeated calls to WCS.wcs.set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to clarify that memory race was occurring because each thread was calling WCS.wcs.set()
in parallel, and not because of repeated calls to wcsset()
.
2a18fcc
to
eb53f85
Compare
eae36e4
to
b594a21
Compare
This pull request is an attempt to fix #16245 - it is just a proof of concept at this point and should not be merged yet, we first need to agree whether this is a reasonable workaround. The issues in #16245 is caused fundamentally by
wcsset
not being thread-safe and being called from different threads. However, these calls are not necessary because the WCS is not changing between all the calls. So the approach in this PR is to keep track of what thewcsprm
was after the last timewcsset
was called, and exit early if it hasn't changed since.Note that using
flag
as set bynote_change
is not sufficient, because users can modify some of the array attributes of the WCS in-place, such as:which won't trigger
note_change
.As also noted in #16409, using
wcsprm_python2c
andwcsprm_c2python
itself is not really thread-safe though that can be addressed separately as it is used in other places too and luckily doesn't seem to actually trigger threading issues in practice. With this PR, the example in #16245 outputs:I need to assess what the performance impact of this PR is.
cc @manodeep