Uploaded image for project: 'Couchbase Server'
  1. Couchbase Server
  2. MB-49284

Performance: Plasma constructors shallow copy huge Config object many times

    XMLWordPrintable

Details

    • Untriaged
    • 1
    • Unknown

    Description

      plasma.go constructors New(), New2(), New3() all accept argument cfg Config, which is a struct, therefore it is being passed by value not by reference. This means each call, and each child this is passed to that does the same thing, makes a new shallow copy of the entire Config struct. By my count this struct currently contains 113 fields, so these copies are doing a lot of extra CPU work and consuming significant stack.

      E.g. a call to New(cfg) triggers the following copies (at least the first 5, and possibly up to 7, assuming none of the great-grandchildren copy it further):

      1. New(cfg) – arg copied on call
      2. New2(cfg, ...) – copied to child call
      3. New3(cfg, ...) – copied to grandchild call
      4. applyConfigDefaults(cfg, ...) – copied to first great-grandchild call
      5. cfg = applyConfigDefaults(...) – and copied again assigning the returned copy back to New3's cfg variable
      6. doRecovery(..., cfg, ...) – copied to second great-grandchild if this call is made
      7. newPlasmaSkeleton(cfg, ...) – copied to third great-grandchild if this call is made

      Attachments

        For Gerrit Dashboard: MB-49284
        # Subject Branch Project Status CR V

        Activity

          kevin.cherkauer Kevin Cherkauer created issue -
          jliang John Liang added a comment -

          This is only called once per instance created. It does not happen very often. Having said that, we could pass in pointer and do the copy until the very last one. Not sure how much CPU it will actually saved though.

          jliang John Liang added a comment - This is only called once per instance created. It does not happen very often. Having said that, we could pass in pointer and do the copy until the very last one. Not sure how much CPU it will actually saved though.
          jliang John Liang made changes -
          Field Original Value New Value
          Assignee John Liang [ jliang ] Dmitriy Kalugin-Balashov [ dmitriy.kalugin-balashov ]
          jliang John Liang made changes -
          Labels plasma
          kevin.cherkauer Kevin Cherkauer made changes -
          Description plasma.go constructors *New(), New2(), New3()* all accept argument *cfg Config*, which is a struct, therefore it is being passed by value not by reference. This means each call, and each child this is passed to that does the same thing, makes a new shallow copy of the entire Config struct. By my count this struct currently contains 113 fields, so these copies are doing a lot of extra CPU work and consuming significant stack.

          E.g. a call to New(cfg) triggers the following copies (at least the first 5, and possibly up to 7, assuming none of the grandchildren copy it further):
           # New({color:#ff0000}*cfg*{color}) – arg copied on call
           # New2({color:#ff0000}*cfg*{color}, ...) – copied to child call
           # New3({color:#ff0000}*cfg*{color}, ...) – copied to grandchild call
           # applyConfigDefaults({color:#ff0000}*cfg*{color}, ...) – copied to first grandchild call
           # {color:#ff0000}*cfg*{color} = applyConfigDefaults(...) – and copied again assigning the returned copy back to New3's cfg variable
           # doRecovery(..., {color:#ff0000}*cfg*{color}, ...) – copied to second grandchild if this call is made
           # newPlasmaSkeleton({color:#ff0000}*cfg*{color}, ...) – copied to third grandchild if this call is made
          plasma.go constructors *New(), New2(), New3()* all accept argument *cfg Config*, which is a struct, therefore it is being passed by value not by reference. This means each call, and each child this is passed to that does the same thing, makes a new shallow copy of the entire Config struct. By my count this struct currently contains 113 fields, so these copies are doing a lot of extra CPU work and consuming significant stack.

          E.g. a call to New(cfg) triggers the following copies (at least the first 5, and possibly up to 7, assuming none of the great-grandchildren copy it further):
           # New({color:#ff0000}*cfg*{color}) – arg copied on call
           # New2({color:#ff0000}*cfg*{color}, ...) – copied to child call
           # New3({color:#ff0000}*cfg*{color}, ...) – copied to grandchild call
           # applyConfigDefaults({color:#ff0000}*cfg*{color}, ...) – copied to first great-grandchild call
           # {color:#ff0000}*cfg*{color} = applyConfigDefaults(...) – and copied again assigning the returned copy back to New3's cfg variable
           # doRecovery(..., {color:#ff0000}*cfg*{color}, ...) – copied to second great-grandchild if this call is made
           # newPlasmaSkeleton({color:#ff0000}*cfg*{color}, ...) – copied to third great-grandchild if this call is made
          Dmitriy.Kalugin-Balashov Dmitriy Kalugin-Balashov made changes -
          Status Open [ 1 ] In Progress [ 3 ]

          Build couchbase-server-7.1.0-1941 contains plasma commit 02cb58c with commit message:
          MB-49284 Send Config by reference (merge conflict fix)

          build-team Couchbase Build Team added a comment - Build couchbase-server-7.1.0-1941 contains plasma commit 02cb58c with commit message: MB-49284 Send Config by reference (merge conflict fix)

          Build couchbase-server-7.1.0-1941 contains plasma commit d7f6e60 with commit message:
          MB-49284 Send Config by reference

          build-team Couchbase Build Team added a comment - Build couchbase-server-7.1.0-1941 contains plasma commit d7f6e60 with commit message: MB-49284 Send Config by reference
          Dmitriy.Kalugin-Balashov Dmitriy Kalugin-Balashov made changes -
          Resolution Fixed [ 1 ]
          Status In Progress [ 3 ] Resolved [ 5 ]
          ritam.sharma Ritam Sharma made changes -
          Labels plasma plasma request-dev-verify

          Fix is cleanup. No extra test needed.

          srinath.duvuru Srinath Duvuru added a comment - Fix is cleanup. No extra test needed.
          srinath.duvuru Srinath Duvuru made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

          People

            Dmitriy.Kalugin-Balashov Dmitriy Kalugin-Balashov
            kevin.cherkauer Kevin Cherkauer
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes

                PagerDuty