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

DcpProducer::encodeItemHotness acquires config mutex on every call

    XMLWordPrintable

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Done
    • 6.0.0, 5.5.0, 5.5.1, 5.5.2, 5.5.3, 6.0.1
    • 6.5.0
    • couchbase-bucket
    • Untriaged
    • Unknown

    Description

      Spotted whilst looking into other perf regressions in the DcpProducer. The encodeItemHotness method is called for every DCP mutation we send. This is an issue as the current implementation reads an engine configuration value on every call. Engine configuration values are guarded behind a mutex which we have to acquire to read/write. This is rather costly, particularly in the case where we have multiple DcpProducers (i.e. multiple nodes). As the current write heavy daily test (the one that I've been looking most closely at) only has 2 nodes, this has definitely slipped under the radar in perf profiles.

      uint8_t DcpProducer::encodeItemHotness(const Item& item) const {
          if (engine_.getConfiguration().getHtEvictionPolicy() == "hifi_mfu") {
              auto freqCount = item.getFreqCounterValue();
              if (supportsHifiMFU) {
                  // The consumer supports the hifi_mfu eviction
                  // policy, therefore use the frequency counter.
                  return freqCount;
              }
              // The consumer does not support the hifi_mfu
              // eviction policy, therefore map from the 8-bit
              // probabilistic counter (256 states) to NRU (4 states).
              return ItemEviction::convertFreqCountToNRUValue(freqCount);
          }
          /* We are using the 2-bit_lru and therefore get the value */
          return item.getNRUValue();
      }
      

      Had a quick chat with Daniel Owen, who said it should be okay if functionality is reduced to allowing high mfu config to change at restart/recreation of a DcpProducer. There is a mechanism currently used in the DcpConsumer to allow some engine config values to change at runtime so will have a quick look at employing that here and will do so if it is relatively easy.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          drigby Dave Rigby added a comment -

          I spoke to Daniel Owen on Friday, and we actually concluded that the dynamic change of LRU mode can probably just be removed now.

          That would solve this problem and also clean up a bunch of other code where we still have the old LRU support present.

          Note we /do/ still need to support connecting to downlevel clusters where supportsHiFiMRU==false, and hence do need to convert the frequency counter to NRU for them.

          drigby Dave Rigby added a comment - I spoke to Daniel Owen on Friday, and we actually concluded that the dynamic change of LRU mode can probably just be removed now. That would solve this problem and also clean up a bunch of other code where we still have the old LRU support present. Note we /do/ still need to support connecting to downlevel clusters where supportsHiFiMRU==false , and hence do need to convert the frequency counter to NRU for them.

          Build couchbase-server-6.5.0-2460 contains kv_engine commit a18a300 with commit message:
          MB-33151: Rename supportsHifiMfu

          build-team Couchbase Build Team added a comment - Build couchbase-server-6.5.0-2460 contains kv_engine commit a18a300 with commit message: MB-33151 : Rename supportsHifiMfu

          Build couchbase-server-6.5.0-3049 contains kv_engine commit 7143c3d with commit message:
          MB-33151: Remove hifi mfu config param

          build-team Couchbase Build Team added a comment - Build couchbase-server-6.5.0-3049 contains kv_engine commit 7143c3d with commit message: MB-33151 : Remove hifi mfu config param

          Closing all invalid, duplicate, won't-fix and User error bugs. Please feel free to reopen

          raju Raju Suravarjjala added a comment - Closing all invalid, duplicate, won't-fix and User error bugs. Please feel free to reopen

          People

            ben.huddleston Ben Huddleston
            ben.huddleston Ben Huddleston
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes

                PagerDuty