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

Investigate freeing keyIndex when closing checkpoint

    XMLWordPrintable

Details

    • Task
    • Status: Resolved
    • Minor
    • Resolution: Won't Do
    • None
    • None
    • couchbase-bucket
    • None

    Description

      Checkpoint expelling tries to reduce the memory used by checkpoints by freeing items all cursors have already "visited". If there are no cursors in the checkpoint, it may instead be dropped as a whole.

      The memory overhead of checkpoints (includes memory allocated for the checkpoint keyIndex) has been (anecdotally) seen to remain "high" after checkpoint expelling has been performed.

      At a glance, the keyIndex appears to serve no purpose once a checkpoint is closed. It may be worth investigating whether the memory tied up in keyIndex could be reclaimed, either upon the checkpoint being closed or checkpoint expelling.

      Attachments

        Issue Links

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

          Activity

            See MB-35889 and MB-36301. When we tried to do this we hit a regression as dropping the checkpoints under the checkpoint manager queue lock was costly. We could swap the indexes and drop them after we unlock the queue lock but need to run perf tests to determine what the benefit of this is.

            ben.huddleston Ben Huddleston added a comment - See MB-35889 and MB-36301 . When we tried to do this we hit a regression as dropping the checkpoints under the checkpoint manager queue lock was costly. We could swap the indexes and drop them after we unlock the queue lock but need to run perf tests to determine what the benefit of this is.
            drigby Dave Rigby added a comment -

            James Harrison / Paolo Cocchi What are your thoughts on this? Given we now destroy Checkpoints via a background task, perhaps this is no longer an issue?

            I note that the description makes the distinction that we could free keyIndex when we change a Checkpoint from Open -> Closed - which obviously happens before a checkpoint is destroyed (and a Checkpoint could exist in the "Closed" state for an extended period - so maybe there's still value in this...?

            drigby Dave Rigby added a comment - James Harrison / Paolo Cocchi What are your thoughts on this? Given we now destroy Checkpoints via a background task, perhaps this is no longer an issue? I note that the description makes the distinction that we could free keyIndex when we change a Checkpoint from Open -> Closed - which obviously happens before a checkpoint is destroyed (and a Checkpoint could exist in the "Closed" state for an extended period - so maybe there's still value in this...?
            paolo.cocchi Paolo Cocchi added a comment - - edited

            True that a checkpoint could stays Closed/Referenced for an extended period if there's any slow cursor around.. But I would expect that at some point (eg, quite soon during heavy load) that situation will lead to dropping the cursor..
            My feeling is that in real scenarios is being quite unlikely to see checkpoint mem issues caused by mem-overhead in the closed checkpoints.
            We do have a potential issue in the Open checkpoints, but we can't do too much on that side (assuming that we want to keep deduplication). We did mitigate that problem though, compared to 7.0 now we are allowed to spread allocations across more checkpoints, so the relative weight of the Open checkpoint is now much lower than before.

            paolo.cocchi Paolo Cocchi added a comment - - edited True that a checkpoint could stays Closed/Referenced for an extended period if there's any slow cursor around.. But I would expect that at some point (eg, quite soon during heavy load) that situation will lead to dropping the cursor.. My feeling is that in real scenarios is being quite unlikely to see checkpoint mem issues caused by mem-overhead in the closed checkpoints. We do have a potential issue in the Open checkpoints, but we can't do too much on that side (assuming that we want to keep deduplication). We did mitigate that problem though, compared to 7.0 now we are allowed to spread allocations across more checkpoints, so the relative weight of the Open checkpoint is now much lower than before.

            It seems unlikely that checkpoint overhead from closed checkpoints will cause major problems, especially with all the recent improvements (CM quota, more checkpoints, parallel destruction and eager removal).

            Incidentally, looking through the list of reverts in the related MBs, I see mention of the memory tracking allocator - I wonder if the regression might also have involved the perf issue recently fixed in this change?

            james.harrison James Harrison added a comment - It seems unlikely that checkpoint overhead from closed checkpoints will cause major problems, especially with all the recent improvements (CM quota, more checkpoints, parallel destruction and eager removal). Incidentally, looking through the list of reverts in the related MBs, I see mention of the memory tracking allocator - I wonder if the regression might also have involved the perf issue recently fixed in this change ?
            drigby Dave Rigby added a comment -

            Incidentally, looking through the list of reverts in the related MBs, I see mention of the memory tracking allocator - I wonder if the regression might also have involved the perf issue recently fixed in this change?

            Quite possibly.

            Thanks both for the comments. At this stage I think we just resolve this issue, given the many changes since this was raised.

            drigby Dave Rigby added a comment - Incidentally, looking through the list of reverts in the related MBs, I see mention of the memory tracking allocator - I wonder if the regression might also have involved the perf issue recently fixed in this change? Quite possibly. Thanks both for the comments. At this stage I think we just resolve this issue, given the many changes since this was raised.

            People

              james.harrison James Harrison
              james.harrison James Harrison
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes

                  PagerDuty