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

update memory management in jointcal

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: jointcal
    • Labels:
      None
    • Story Points:
      8
    • Sprint:
      DRP S17-5, DRP S17-6
    • Team:
      Data Release Production

      Description

      jointcal currently uses a combination of raw pointers and a custom reference-counted smart pointer class, CountedRef (similar to boost::intrusive_ptr). The code needs to be modified to use a combination of shared_ptr (most code), unique_ptr local-scope variables and factory functions, and weak_ptr (at least some will be necessary to avoid cycles in some of the more complex data structures). As part of this work, we'll also have to remove a lot of inheritance from RefCounted, which is part of the CountedRef implementation.

      This ticket looks like it will require a lot of work, because we'll have to be careful about every conversion to avoid cycles and memory leaks. Nevertheless, I think it will be necessary to do this conversion before attempting any other major refactoring, as I'm worried that having a newcomer make changes to the codebase without first making the memory management less fragile could be very dangerous.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            That would be in the scope of this ticket, but I was hoping to get it with minimal work as part of DM-9187, even though it was out of scope. Given that I spent a good part of today on it already (and it's a bonus feature, not a necessary thing), I'll just keep waiting on this ticket.

            Show
            Parejkoj John Parejko added a comment - That would be in the scope of this ticket, but I was hoping to get it with minimal work as part of DM-9187 , even though it was out of scope. Given that I spent a good part of today on it already (and it's a bonus feature, not a necessary thing), I'll just keep waiting on this ticket.
            Hide
            jbosch Jim Bosch added a comment -

            Ok. I wasn't sure how important it was to expose those objects to Python. FWIW, I think using a custom smart pointer in pybind11 will be significantly easier than the rest of this ticket, but if it's not a high priority this ticket will indeed solve the problem for you.

            Show
            jbosch Jim Bosch added a comment - Ok. I wasn't sure how important it was to expose those objects to Python. FWIW, I think using a custom smart pointer in pybind11 will be significantly easier than the rest of this ticket, but if it's not a high priority this ticket will indeed solve the problem for you.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Now uses unique_ptr and shared_ptr for memory management.
            I checked with valgrind and apart from the (unrelated and separately ticketed) leak in wcslib all seems to be ok.
            Memory usage profile also does not show any significant difference.

            I restricted my changes to direct conversions. It could still benefit from some extra cleanup related to pointer usage though. Specifically:

            • use std::vector instead of raw arrays (now encapsulated in unique_ptr);
            • refactor to get rid of frequent (dynamic) casts, this seems possible in pretty much all cases.

            However, for this ticket I consider that to be out of scope.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Now uses unique_ptr and shared_ptr for memory management. I checked with valgrind and apart from the (unrelated and separately ticketed) leak in wcslib all seems to be ok. Memory usage profile also does not show any significant difference. I restricted my changes to direct conversions. It could still benefit from some extra cleanup related to pointer usage though. Specifically: use std::vector instead of raw arrays (now encapsulated in unique_ptr ); refactor to get rid of frequent (dynamic) casts, this seems possible in pretty much all cases. However, for this ticket I consider that to be out of scope.
            Hide
            Parejkoj John Parejko added a comment -

            Thank you for the review. See my comments on the PR. I dug around a bit and I'm fine with the removal of the refStar->fittedStar pointer.

            Show
            Parejkoj John Parejko added a comment - Thank you for the review. See my comments on the PR. I dug around a bit and I'm fine with the removal of the refStar->fittedStar pointer.
            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Merged after implementing requested changes.

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Merged after implementing requested changes.

              People

              Assignee:
              pschella Pim Schellart [X] (Inactive)
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              John Parejko
              Watchers:
              Dominique Boutigny, Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen, Pierre Astier, Pim Schellart [X] (Inactive), Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.