Clarify QPACK eviction races

[afrind]

Fixes #1129

[martinthomson]

I'm fairly stupid during meetings that I pay attention to, but I didn't follow this last paragraph very well. Smallest implies small numbers, but I think that you mean smallest "base" (which is the inverse of indices).

I probably don't really understand what you are trying to say, so consider tweaking for clarity. That might mean more text.

[afrind]

Sorry, I meant the smallest absolute index. I wouldn't call it a base, since I think of a base as an absolute index that you encode against. I'm guessing that alone isn't enough to clarify?

I think of draining space as space in the table occupied by valid entries that the encoder chooses not to use.

Here's the little snippet of code that I use to implement, where minUsable_ represents my draining index:

  while (capacity_ - bytes_ + drainedBytes_ < table_->minFree() &&
         minUsable_ <= baseIndex_) {
    VLOG(5) << "Draining absolute index " << minUsable_;
    drainedBytes_ += (*table_)[absoluteToInternal(minUsable_++)].bytes();

  }

[MikeBishop]

SHOULD implies that the encoder has a different option which is appropriate in certain circumstances. What else could the encoder do, and when would it choose that? (I don't think it does, in which case I'd just skip the normative language here and say that's what the encoder does.)

[afrind]

ok

[MikeBishop]

I'm not sure if "Free Space" is necessarily the correct thing here, at least in a steady state (which is the only time eviction is an issue). I suppose it's sometimes possible to insert something small without evicting anything, but I'm not sure if that's relevant.

[afrind]

There's usually some free space because it's not possible to evict a partial header. Maybe I could make it look smaller? The text talks about the sum of free space and draining space so I wanted to include it in the picture.

[martinthomson]

The point here is not that there might be some free space (there is almost always free space because header fields are not necessarily a neat fit for the available space), but that the encoder needs to keep its hands off those entries that are close to eviction. Eviction-prone is misleading, because it implies that there is a probability of eviction.

I think that this map is misleading though. There are three important classifications here:

• Entries that are near eviction, such that new insertions might be blocked by outstanding references to those entries.
• Entries that are recently added (i.e., those that are "vulnerable"), such that references to them might cause the receiver to block.
• Other entries that are acknowledged, but not near eviction.

The line between those that are "vulnerable" and acknowledged is crisp. The point here is that the line between near-eviction and merely acknowledged is fuzzy and subject to heuristics.

I think that you also want to say that keeping the near-eviction index in the range of acknowledged entries will prevent perverse behaviour. If the two ever cross, you can get duplication on every header block, which causes unnecessary churn. (It's also possible to get that without the two crossing with a naive implementation, but I don't think that we need to get into that too much.)

[afrind]

I'm not sure I agree that the draining index always needs to be in the acknowledged range. Consider the case where I insert a full table's worth of headers and none have yet been acknowledged. An encoder will still want to stop referencing the oldest ones so they can be evicted soon, even if it causes duplication. For the purposes of this section, I'm not sure I want to add the 'acknowledged index' to the picture.

[MikeBishop]

I'm with @afrind. The pathological case I think you're describing is an encoder that won't / isn't permitted to block deciding that it should duplicate an old entry, but still can't use either one. If it doesn't duplicate it more than once, that should be okay.

[martinthomson]

Has this been superseded by #1357?

[martinthomson]

@MikeBishop, ping.

[MikeBishop]

My opinion is no. But as I'm neither the author of the PR nor the editor for this doc, let's redirect that ping to @afrind. Want to rebase and merge this?

[afrind]

Yes, I'll come back to this. It was low pri relative to the other design changes I wanted to land for -01

[martinthomson]

I have tried to be very specific in my suggestions, but feel free to take or leave.

[martinthomson]

"When the encoder encounters a blocked eviction, ..." involves a logical leap, or an assumption about the structure of encoders that hasn't been explained. We all assume that encoders emit new table modifications as the result of processing header blocks, but that isn't necessary.

Perhaps: "An encoder MUST NOT insert an entry into the dynamic table (or duplicate an existing entry) if that would cause an entry with unacknowledged references to be evicted. For header blocks that might rely on the newly added entry, the encoder can use a literal representation and maybe insert the entry later."

[martinthomson]

The title here might be better as "Blocked Dynamic Table Insertions". Evictions don't get blocked, they are the immovable object here. It's more the case that insertions are what get blocked.

[martinthomson]

"To ensure that the encoder is not prevented from adding new entries, the encoder can avoid referencing entries that are near the point at which they would be evicted. Rather than ..."

[martinthomson]

This has to be a single line. Also, check whether You Want Title Case or Sentence case. I think that we use Title Case elsewhere.

[martinthomson]

"As new entries are inserted, the encoder increases the draining index to maintain the section of the table that it will not reference. Draining entries - entries with an absolute index lower than this the draining index - will not accumulate new references. The number of unacknowledged references to draining entries will eventually become zero, making the entry available for eviction."