Avoid calling wcsset if the wcsprm is unchanged

[astrofrog]

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 the wcsprm was after the last time wcsset was called, and exit early if it hasn't changed since.

Note that using flag as set by note_change is not sufficient, because users can modify some of the array attributes of the WCS in-place, such as:

wcs.wcs.cdelt[1] = 1

which won't trigger note_change.

As also noted in #16409, using wcsprm_python2c and wcsprm_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:

Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0
Mismatching elements: 0

I need to assess what the performance impact of this PR is.

cc @manodeep

[github-actions]

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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[github-actions]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[astrofrog]

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.

[manodeep]

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

[manodeep]

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.

[manodeep]

[Adding what Tom and I discussed over slack] This fix is a trade-off in ensuring that wcsset is only called when necessary - the trade-off being that wcsset will be called even when a changed field does not require a wcsset call. This fix also ensures that the solution continues to work in the future when a new field might be introduced where changed values require a wcsset call.

(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 wcsset would then have to compare the previously stored values with the current contents, potentially as a memcmp assuming no padding bytes are involved (i.e., structs containing padding bytes between fields) are involved. Such a correct fix would be more complex and more invasive than Tom's fix)

[manodeep]

Here are the relevant fields that require a wcsset when changed (taken from these lines)

*       - 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.

[manodeep]

With wcslib v8.3, there is a new function int wcsenq(const struct wcsprm *wcs, int enquiry) which when invoked as wcsenq(wcs, WCSENQ_CHK) will return 0 when the previous and current checksums don't match signalling that wcsset() needs to be called again. This can (hopefully) detect cases where some values of an array/list parameter (i.e., where the getters are invoked, rather than the setters) have been changed and that wcsset() needs to be called again.
`

[astrofrog]

@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)

[astrofrog]

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?

[astrofrog]

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.

[manodeep]

Made two minor comments - looks good to me

[manodeep]

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)?

[astrofrog]

Just out of curiosity, which part of wcsenq is not thread-safe?

[manodeep]

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

[manodeep]

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().