fix: Add time slicing to splitstore purging to reduce lock congestion

[fridrik01]

Related Issues

See #11251

Context

When splitstore is enabled we perform compaction every 7h30m. During this compaction (which takes several minutes on my fast SSD) there is relatively small purging step which takes approx 15sec. The purging step chunks the cids which should be purged and operates on them in blocks. In each block it acquires an exclusive lock which it shares with other I/O operation (for example FVM when applying messages).

So when purging happens, other operations like MpoolSelect block for the whole 15sec duration when it needs to compute tipset state (TipSetState).

Proposed Changes

I added a simple time slicing to the purging step which only tries to purge for 1 sec, while pausing for 1 sec until it is done. This effectively makes the purge 2x slower, but I don't think we care so much as it only happens every 7h30m and purging is already relatively fast compare the the whole splitstore compaction.

Test plan

Using the same testing method as described in this comment I observed with this fix that during splitstore compaction the whole TipSetState went down from 15sec to ~2sec.

Additional Info

See #11251 (comment)

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[Stebalien]

We likely either need to:

1. Take some kind of lock when computing tipsets (and/or mining blocks). 1s isn't quite enough to compute a tipset so we'll end up pausing multiple times.
2. Pause longer. IMO, there's no rush here unless I'm missing something.

That's not to say that the current patch won't help. Even if we only pause for 1s, that means we'll be able to spend 50% of the time computing the tipset which means it'll take ~6s on average instead of 3. But I'd still consider pausing for longer.

[Stebalien]

That's not to say that the current patch won't help. Even if we only pause for 1s, that means we'll be able to spend 50% of the time computing the tipset which means it'll take ~6s on average instead of 3. But I'd still consider pausing for longer.

I.e., I'd consider pausing for 4s every 1s, or something like that. That gives us 20% utilization, which means it should only make blocks take 20% longer.

Ideally we'd look at datastore metrics and determine that the datastore is "busy". But that's more complicated.

[Stebalien]

LGTM but this one needs an approval from @arajasek.

[arajasek]

I don't see any harm in doing this, but I'm not sure I fully believe the logic here.

When splitstore is enabled we perform compaction every 7h30m. During this compaction (which takes several minutes on my fast SSD) there is relatively small purging step which takes approx 15sec. The purging step chunks the cids which should be purged and operates on them in blocks. In each block it acquires an exclusive lock which it shares with other I/O operation (for example FVM when applying messages).

We're saying that the purging step takes 15 seconds, but operates in batches. The batch size is 16k, which seems relatively small to me. We take the lock for each batch.

So when purging happens, other operations like MpoolSelect block for the whole 15sec duration when it needs to compute tipset state (TipSetState).

In that case, we shouldn't be hogging the lock for the entire 15s, no? We're aggressively trying to grab it between batches, but we should still be releasing the lock for other operations in between batches. Is it the case that a singly batch is taking 15s?

[arajasek]

See comment about the premise itself. If we're landing, let's make the "work" time and "sleep" time constants (no need to be configurable yet).

[arajasek]

4 * elapsed? Do we not want 4 * time.Second here?

[fridrik01]

if we schedule slightly over second, this means that we sleep a bit longer guaranteeing a 20% utilization

[arajasek]

Yup, that makes sense.

[fridrik01]

I don't see any harm in doing this, but I'm not sure I fully believe the logic here.

When splitstore is enabled we perform compaction every 7h30m. During this compaction (which takes several minutes on my fast SSD) there is relatively small purging step which takes approx 15sec. The purging step chunks the cids which should be purged and operates on them in blocks. In each block it acquires an exclusive lock which it shares with other I/O operation (for example FVM when applying messages).

We're saying that the purging step takes 15 seconds, but operates in batches. The batch size is 16k, which seems relatively small to me. We take the lock for each batch.

So when purging happens, other operations like MpoolSelect block for the whole 15sec duration when it needs to compute tipset state (TipSetState).

In that case, we shouldn't be hogging the lock for the entire 15s, no? We're aggressively trying to grab it between batches, but we should still be releasing the lock for other operations in between batches. Is it the case that a singly batch is taking 15s?

Yes we release the lock between batches but when applying messages we fight for that lock independently for each message so if purging takes 15sec we end up trying to acquire the lock for these messages until the whole purging step is done.

[arajasek]

@fridrik01 Aaah, the lock contention is for every single message application. Got it, that makes complete sense. Thank you!

[arajasek]

Make time.Second a constant, but LGTM!