Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:None
-
Story Points:2
-
Epic Link:
-
Sprint:DRP F18-4
-
Team: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
- blocks
-
- Done
Activity
| Risk Score | 0 |
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
| Reviewers | Krzysztof Findeisen [ krzys ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
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.
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. Krzysztof Findeisen
made changes -
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
Thanks for the very helpful review, Krzysztof Findeisen. 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).
| Sprint | DRP F18-4 [ 774 ] |
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
Krzysztof Findeisen, mind taking this review of a small refactoring of some afw::image::Mask internals?
PR is at https://github.com/lsst/afw/pull/386.