Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-81848

Make decoration construction eager again

    • Service Arch
    • Fully Compatible
    • Service Arch 2023-11-27, Service Arch 2023-12-11, Service Arch 2023-12-25, Service Arch 2024-01-08, Service Arch 2024-01-22, Service Arch 2024-02-05

      Based on my testing, the change to make some decorations use LazyInit slowed down operations rather than making them faster. I think this is caused by a few things:

      • The only things that were made lazy were things that are relatively cheap to value-construct such as unique_ptr (setting a single pointer to null), optional (setting a single bool to false), BSONObj (setting one pointer to null and another to a fixed value), etc. These types are also cheap to destroy when left in their freshly constructed state, generally just being a check for null/zero/false and returning since it will be.
      • While it made construction cheaper, it also made access more expensive by both adding an extra indirection in the critical path to get the offset into the buffer rather than storing it in the Decoration object (this didn't seem to be required by the change, as far as I can tell) and adding an extra acquire-load to check if the data is initialized (which requires fencing on arm, and prior to gcc-13/clang-16 is silently upgraded to seq-cst). T
      • This added expense is distributed throughout the code so it is harder to find and address vs it being centralized when making the OperationContext
      • We also made decoration on long-lived objects like the ServiceContext lazy, even though we would much rather pay any cost up for them front than on every access. Client may be a bit fuzzier, but I would consider it long-lived for this purpose as well.

      Given the above, I think it is best to simply revert this commit and return to eager initializing. I think rather than trying to automatically lazify construction of many decorations, it makes more sense to selectively target specific Decorations that are rarely used and expensive to construct. This would also let use choose whether they need to be thread-safe or not and just use a boost::optional if they don't. I don't have any specific suggestions for Decorations to lazify since once that commit is reverted, constructing the decorations on OperationContext doesn't seem to be expensive enough to worry about (constructing the LockState and RecoveryUnit are, but they aren't decorations).

      While in that code, there are a few other changes that may make sense, but I think they are less critical.

      • Given that we are already zero-filling the whole buffer, we could use default-init (new (p) T;) rather than value-init (new (p) T{};) in getConstructorFn()
      • We can assume that zero-init is good enough for some types that aren't trivial init, such as boost::optional (safer since we control the definition) and std::unique_ptr<T> with no custom deleter (slightly more risky since the definition is provided by the toolchain, however it is hard to imagine a practical implementation where zero filling isn't correct, and our tests would explode if it wasn't)
      • Do something about CurOpStack. On arm64 is acounts for 3456 bytes out of the 5752 bytes (60%!) in OperationContext's decoration buffer. Given that we zero init the whole thing we are paying for all of that all the time. We should verify if it is all needed all the time, or if it would be better to move some of it out of line.

            Assignee:
            alex.li@mongodb.com Alex Li
            Reporter:
            mathias@mongodb.com Mathias Stearn
            Votes:
            0 Vote for this issue
            Watchers:
            12 Start watching this issue

              Created:
              Updated:
              Resolved: