Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-15563

Refactor Mask global state and make it thread-friendly

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • afw
    • None
    • 2
    • DRP F18-4
    • Data Release Production

    Description

      This is a spin-off from DM-15500: I needed to move some code out of Mask.cc; in order to do that, I needed to refactor it a bit, and from there it snowballed into a near rewrite of one corner of Mask's implementation, including adding a mutex to guard access to global state.  Since that brings it pretty far from DM-15500's original goal, it makes sense to make that a separate review and merge.

      The end result is intended to be entirely backwards compatible with the old Mask API and behavior, and that takes some other cleanup work off the table.  But I think the result is still a much safer and easier-to-understand implementation.

      Attachments

        Issue Links

          Activity

            jbosch Jim Bosch added a comment -

            krzys, mind taking this review of a small refactoring of some afw::image::Mask internals?

            PR is at https://github.com/lsst/afw/pull/386.

            jbosch Jim Bosch added a comment - krzys , mind taking this review of a small refactoring of some afw::image::Mask internals? PR is at https://github.com/lsst/afw/pull/386 .

            Looks good, except for the use of weak_ptr and the requirements it imposes on any code that reads allMaskDicts. I think the original strategy was simpler, and therefore safer.

            Unfortunately, I will be out of contact next week. Since I don't think any of my concerns are serious enough to block merging, I've marked the ticket as reviewed.

            krzys Krzysztof Findeisen added a comment - Looks good, except for the use of weak_ptr and the requirements it imposes on any code that reads allMaskDicts . I think the original strategy was simpler, and therefore safer. Unfortunately, I will be out of contact next week. Since I don't think any of my concerns are serious enough to block merging, I've marked the ticket as reviewed.
            jbosch Jim Bosch added a comment - - edited

            Thanks for the very helpful review, krzys.  I was not unhappy with the state of this before the review, but I like it a lot better now.  One big change you may not have expected: I went ahead and made GlobalState itself thread-safe, moving all of the operations that needed to lock the mutex into instance methods.  That cleaned up a ton - while there's a bit of repetition now in that MaskDict has a number of methods that just trivially forward to GlobalState's implementations, it made Lock unnecessary and it naturally resolved a lot of your concerns about the details of the thread-safety logic.

            I know you're not around this week, but if you are interested in and able to take another look, I'm not in a hurry to merge this (though I do hope to by the end of the week - I just need to merge it before DM-15500, which is not done yet).

            jbosch Jim Bosch added a comment - - edited Thanks for the very helpful review, krzys .  I was not unhappy with the state of this before the review, but I like it a lot better now.  One big change you may not have expected: I went ahead and made GlobalState  itself thread-safe, moving all of the operations that needed to lock the mutex into instance methods.  That cleaned up a ton - while there's a bit of repetition now in that MaskDict has a number of methods that just trivially forward to GlobalState 's implementations, it made Lock  unnecessary and it naturally resolved a lot of your concerns about the details of the thread-safety logic. I know you're not around this week, but if you are interested in and able to take another look, I'm not in a hurry to merge this (though I do hope to by the end of the week - I just need to merge it before DM-15500 , which is not done yet).

            People

              jbosch Jim Bosch
              jbosch Jim Bosch
              Krzysztof Findeisen
              Jim Bosch, Krzysztof Findeisen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.