Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow PRIORITY frames referring to placeholders exceeding SETTING_NUM_PLACEHOLDERS #2761

Conversation

kazuho
Copy link
Member

@kazuho kazuho commented May 31, 2019

At the moment, it is the client's obligation to build a prioritization tree by respecting the server-advertised value of SETTINGS_NUM_PLACEHOLDERS.

That seems like an unnecessary complexity. We assume servers to provide sensible amount of space for placeholders (i.e., SHOULD). Then, why do the clients need to be forced to implement complex logic to handle servers not complying to the recommendation?

This PR brings back the design we have in HTTP/2; i.e. give clients the freedom to express a prioritization tree that they need regardless of what the servers advertise using the SETTINGS_NUM_PLACEHOLDERS settings. The settings is changed to a purely advisory value; clients that do care are allowed to respect the value.

Maybe closes #2734, #2753.

A PRIORITY frame MAY reference a Placeholder ID that is equal to or greater than
the server's advertised value of the `SETTINGS_NUM_PLACEHOLDERS` settings. The
server SHOULD ignore PRIORITY frames that specify such placeholders as the
prioritized element. The server SHOULD use the root of the tree as the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want s/prioritized element/element dependency. Such that if a stream is assigned as dependent on a placeholder ID greater than SETTINGS_NUM_PLACEHOLDERS, then it becomes dependent on root.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM, you say that in the next sentence.

Perhaps if you reorder the text to make the subject of each sentence come earlier, it will be easier to read it correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LPardue Thank you for the comment. I've changed the flow of the sentences in 4579ada. PTAL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks better now.

@LPardue
Copy link
Member

LPardue commented May 31, 2019

A general comment on this is that things might get racy depending on how things get resolved with the initial value of SETTINGS_NUM_PLACEHOLDERS. If the default is low and a SETTINGS frame would increase it, then the client that attempts to set placeholder priority too eagerly may not get the result they expect.

In practice what we might see is that different servers implement some internal limit to num placeholders and manage the tree that way, settling on some value that works well for most clients.

I think that is the spirit of this change. This change allows some looser behaviour and unlike HTTP/2 phantom streams, doesn't tie up stream credits for prioritisation structures.

@ghedo
Copy link
Member

ghedo commented May 31, 2019

This change doesn't make a lot of sense to me TBH. If the client is allowed to ignore the server setting, then there's no point to that setting.

If the client can't adapt its prioritization scheme based on the number of placeholders allowed by the server and proceeds to use more placeholders, then it is likely to end up effectively disabling prioritization for the connection completely (if the server does set a limit it probably means it won't respect the priorities that use more than the placeholders allowed, otherwise it would set the limit to a very high value).

So in practice servers will just end up setting their internal limits and the setting to either whatever value popular clients need, making the setting meaningless, or still set it to a lower value effectively disabling prioritization with some clients.

Anyway, TBH at this point I'd rather spend time exploring the discussion about limiting or even removing placeholders as we talked about in London (including talking with HTTPbis) rather than try to hack around all of these problems.

@LPardue
Copy link
Member

LPardue commented May 31, 2019

This change puts placeholders in the same camp as MAX_HEADER_LIST_SIZE, an advisory value. I also agree with your argument there, a limit that is ignored is kinda useless.

What this change does allow is a fire and forget approach from the client. It gracefully fails because the client can't reliably validate that any prioritisationinformation was adhered to.

@ghedo
Copy link
Member

ghedo commented May 31, 2019

Sure, but clients either care about priorities, in which case they should be built such that they can deal with whatever placeholders limit instead of risking having priorities silently ignored, or they don't, in which case they might as well not send priorities at all.

@LPardue
Copy link
Member

LPardue commented May 31, 2019

I guess one argument is that since prioritisation is a hint anyway, the clients are already able to handle being silently ignored.

However, to support your argument, it may be better to ignore all priority tree requests rather than fail e.g. half of them. And because the server doesn't know how many placeholders the client wants, it can never guess how many would be enough, ever.

@kazuho
Copy link
Member Author

kazuho commented Jun 4, 2019

@ghedo

TBH at this point I'd rather spend time exploring the discussion about limiting or even removing placeholders as we talked about in London (including talking with HTTPbis) rather than try to hack around all of these problems.

It is my view that we should continue refining the prioritization scheme of HTTP/3, though I would not argue for suppressing any other discussion.

The concern of redesigning the prioritization scheme is that the standardization process might get delayed. We do not have a working group consensus on what the goal of the design would be. Compared to that, making changes to lessen the divergence from the prioritization scheme of HTTP/2 is in line with the charter, it is a goal that does not move, and we know how to reintroduce the exclusive bit (see #2754). This PR is a small change that we might adopt alongside other changes that lessen the divergence.

@kazuho
Copy link
Member Author

kazuho commented Jun 4, 2019

@LPardue

However, to support your argument, it may be better to ignore all priority tree requests rather than fail e.g. half of them. And because the server doesn't know how many placeholders the client wants, it can never guess how many would be enough, ever.

Yeah. I think that the anticipation behind this PR is that a server would either support no placeholders at all, or be capable of handling enough number of placeholders. FWIW, Firefox uses 4 "placeholders" in HTTP/2. I do not think that there would be a reason for a HTTP/3 with support for placeholders to support something below 4.

Maybe we should change the recommendation of SETTINGS_NUM_PLACEHOLDERS that state "this value SHOULD be set to a non-zero value by servers" to "this value SHOULD be set to a value equal to or greater than four" or something alike.

@rmarx
Copy link
Contributor

rmarx commented Jun 5, 2019

Yeah. I think that the anticipation behind this PR is that a server would either support no placeholders at all, or be capable of handling enough number of placeholders. FWIW, Firefox uses 4 "placeholders" in HTTP/2. I do not think that there would be a reason for a HTTP/3 with support for placeholders to support something below 4.

@kazuho do you have a source for this? afaik, it uses 6...

I agree that current browser approaches do not require many placeholders, but as also included in Ian's presentation in London, if you want to simulate something like @patmeenan's scheme with placeholders, you need 4 placeholders per priority-level (of which that scheme uses a default of 3, so 12 placeholders minimum total).

I share's @ghedo's views that making this an advisory SETTING would not mean much in practice if not coupled with a minimum amount of placeholders servers MUST support. I'm more for requiring at least X placeholders (X > 32 in my opinion).

placeholder, the server SHOULD discard the frame without making any change to
the priority tree. When receiving a PRIORITY frame with its element dependency
set to such a placeholder, the server SHOULD update the priority tree with the
root of the tree being the dependency instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to #2690, should the default be the orphan or the root? Root makes a bit more sense, since this element is clearly something the client wants to explicitly prioritize. On the other hand, this is a bit dirty, since with the orphan placeholder, nothing else gets explicitly added to the root anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is debatable.

The reason I've chosen "root" as the starting point of this PR is because it is the behavior defined in HTTP/2; I think we should try to retain similarity if possible (unless / until we decide to adopt a very different design). Note that orphan placeholder is still useful regardless of this; the placeholder minimizes the negative impact when PRIORITY frames on the control stream gets lost.

Hopefully, this point becomes a less of an issue once we recommend some sensible minimum for SETTINGS_NUM_PLACEHOLDERS; essentially guiding servers to support either none or at least 32 (or something alike).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the orphan makes more sense -- just because the client is assigning a priority doesn't imply that it's assigning a high priority. The implication would be that clients should build the priority tree with higher-priority placeholders having a lower placeholder ID. At some point, the placeholder IDs cross the threshold of what the server supports, and everything in that tree region devolves to "after everything else." That seems like a reasonable degradation path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implication would be that clients should build the priority tree with higher-priority placeholders having a lower placeholder ID.

This sounds like a good way of thinking. I agree that providing such a guidance is a better than having no guidance at all.

OTOH, That creates a dependency on #2690. @MikeBishop, should we wait for the merger of #2690 before updating the text?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an easy solution to that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am impressed! 18c8303 changes the dependency to the orphan placeholder.

@kazuho
Copy link
Member Author

kazuho commented Jun 6, 2019

@rmarx Thank you for the comments. I see several folks argue for having lower bound for SETTINGS_NUM_PLACEHOLDERS; I've updated the PR to say "at least 32".

It is still a SHOULD rather than a MUST, based on the perception that changing it to a MUST just incentivizes the servers to lie about it's capability, which reduces the usefulness of SETTINGS_NUM_PLACEHOLDERS.

PS. And yes, Firefox uses 6 placeholders, not 4. Thank you for the correction.

draft-ietf-quic-http.md Outdated Show resolved Hide resolved
draft-ietf-quic-http.md Outdated Show resolved Hide resolved
draft-ietf-quic-http.md Outdated Show resolved Hide resolved
placeholder, the server SHOULD discard the frame without making any change to
the priority tree. When receiving a PRIORITY frame with its element dependency
set to such a placeholder, the server SHOULD update the priority tree with the
root of the tree being the dependency instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the orphan makes more sense -- just because the client is assigning a priority doesn't imply that it's assigning a high priority. The implication would be that clients should build the priority tree with higher-priority placeholders having a lower placeholder ID. At some point, the placeholder IDs cross the threshold of what the server supports, and everything in that tree region devolves to "after everything else." That seems like a reasonable degradation path.

@@ -1335,8 +1343,8 @@ The following settings are defined in HTTP/3:
: The default value is unlimited. See {{header-formatting}} for usage.

SETTINGS_NUM_PLACEHOLDERS (0x9):
: The default value is 0. However, this value SHOULD be set to a non-zero
value by servers. See {{placeholders}} for usage.
: The default value is 0. However, servers SHOULD set this value to 32 or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, 32 is a large number. If we keep exclusive prioritization, then Chrome will probably continue to use that, and Firefox doesn't use this many, so why such a high limit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because we use 6 now, doesn't mean that we'll keep that scheme. We're currently looking at ways to improve prioritization and placeholders are seen as being very useful in doing that. It's very early yet, but it might be that more are found to be useful.

And 32 isn't a big number. You need quite a few more zeros to qualify, I'd think.

Copy link
Member Author

@kazuho kazuho Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I do not care much, because the amount of state required by each placeholder is much less than that required by each HTTP stream (which includes send buffers). We recommend servers supporting at least 100 concurrent HTTP streams, so 32 placeholders can be a SHOULD.

That said, I think I might prefer choosing a number that is a divisor of 100 (e.g., 20, 25). I do not think we should go to 50, because I do not think we need one placeholder for every two streams.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Powers of two seem more natural in a protocol, but I am similarly uncaring about the specific number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also suggested 32 because the "power of two" feeling. Otoh, I was thinking about Patrick Meenan's setup, which requires 4 placeholders per priority level in the current scheme, allowing for 8 priority levels if the minimum number of placeholders is 32. 8 is more than e.g., the 5 chrome is using at the moment, but seems like a good number (see also SPDY priority levels).

I just want to prevent us setting this to a too low a number (e.g., 10), since that would counteract the purpose of setting a lower bound imo.

Copy link
Member Author

@kazuho kazuho Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think it makes more sense to use a number that is defined in relation to the number of concurrent requests we suggest to support, I do not think that the current value of 100 is correct (see #2787).

Considering that, I am fine with seeing this PR merged with 32 being the advice, and revisiting the advisory value if necessary.

draft-ietf-quic-http.md Outdated Show resolved Hide resolved
@ianswett
Copy link
Contributor

As I've stated elsewhere, I'm not a fan of this because I'd like placeholders to be optional and be able to enforce this, in an effort to keep H3 as close to H2 codewise.

@kazuho
Copy link
Member Author

kazuho commented Jun 12, 2019

@ianswett

As I've stated elsewhere, I'm not a fan of this because I'd like placeholders to be optional and be able to enforce this, in an effort to keep H3 as close to H2 codewise.

Could you please elaborate a bit more? While I can see why you argue for "placeholders to be optional and be able to enforce this," I do not see why doing so makes H2 and H3 closer.

H2 allows client to create placeholders without regard to what server actually does, and this is about bringing back that behavior.

@ianswett
Copy link
Contributor

From a functionality perspective, I agree this brings H3 inline with H2, but it uses a new mechanism to achieve functional parity. As we've discussed, a new mechanism seems necessary in H3, but it is new nonetheless.

@kazuho
Copy link
Member Author

kazuho commented Jun 14, 2019

@ianswett

From a functionality perspective, I agree this brings H3 inline with H2, but it uses a new mechanism to achieve functional parity. As we've discussed, a new mechanism seems necessary in H3, but it is new nonetheless.

Yeah. Granted that placeholders are new in HTTP/3 and therefore they can be enforced as optional, I do not see why they need to be.

I think our experience in HTTP/2 tells us that clients can specify different weights for requests so that the bandwidths would be distributed based on the weights when the server does not support placeholders at all. I'd assume that most if not all clients would use that same tree if we enforce them to not use placeholders when SETTINGS_NUM_PLACEHOLDERS is set to zero.

To rephrase, I do not think there would be a benefit in terms of performance by having this enforcement, for the majority of our use case.

Compared to that, the complexity being introduced by the enforcement is a burden. Not only because it requires clients to build different priority tree based on the server's settings, but because the client needs to wait for the server's SETTINGS to arrive before it can decide the type of the priority tree to use. For the server, the complexity is the same regardless of the approach; it's either throwing away PRIORITY frames that refer to out-of-bound placeholders or sending connection errors.

Therefore, while placeholders can be considered optional, I do not think it justifies the servers enforcing the limit.

PS. Or is your problem that SETTING_NUM_PLACEHOLDERS to zero is not RECOMMENDED? If so, I think we could change to the recommendation from "at least 32" to "zero or at least 32". As stated above, setting I do not think setting it to zero would be an issue. It set to some wired small value (like 3, which is below the number of placeholders Firefox uses) is the case where we'd see performance issues.

@ianswett
Copy link
Contributor

Certainly if you said zero or at least 32, that would make placeholders more optional, so I'd have a preference for that.

@MikeBishop
Copy link
Contributor

MikeBishop commented Jun 14, 2019

But when it's set to zero, clients no longer have the H2-style option of easy zombie streams to fulfill the role of placeholders, so it's a regression from H2. Sure, Firefox could have Chrome's behavior as a fallback option for servers that say zero, but I don't see that as necessarily being a good thing for the spec to RECOMMEND.

In my opinion, "RECOMMENDED to be 32, allowed to be zero" communicates the right incentives.

@kazuho
Copy link
Member Author

kazuho commented Jun 18, 2019

@MikeBishop @ianswett d07c072 goes the middle round. WDYT?

@ianswett
Copy link
Contributor

That works for me.

@ianswett ianswett added the -http label Jun 18, 2019
@MikeBishop
Copy link
Contributor

Not an issue, just an observation. #2690 added text saying that the client can neither reprioritize nor refer to the orphan placeholder. With this change, we've made it really easy to refer to the orphan placeholder -- any large-numbered placeholder does. (And if you misfire and pick a placeholder that actually could exist, if you've never prioritized it, its parent will be the orphan placeholder, so same thing.)

@MikeBishop
Copy link
Contributor

Never mind -- either I imagined it, or it's already changed by this PR. Either way, it's fine.

draft-ietf-quic-http.md Outdated Show resolved Hide resolved
draft-ietf-quic-http.md Outdated Show resolved Hide resolved

Servers are RECOMMENDED to support at least 32 placeholders. Those that do not
SHOULD refrain from supporting placeholders at all, setting
`SETTINGS_NUM_PLACEHOLDERS` to zero.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this weird. I would strike the second sentence entirely and consider replacing it with justification for 32.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the outcome of the discussion (see above). The intent is:

  • recommend providing at least 32 placeholders
  • acknowledge that 0 is a sensible value when that not possible

The rational is to make it possible for a client to create a priority tree that works reasonably well, assuming that it uses no more than 32 placeholders in these two conditions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand that, but two chained SHOULD statements is hard to follow.

Mostly, I just don't agree with the conclusion of that discussion. If I want to provide 24, that is perfectly fine, and it might be enough for most clients. Pushing me toward advertising zero doesn't help at all.

So I don't agree with the idea that pushing people toward 0 is needed. They can make that decision on their own. And it's the default, so there is plenty of encouragement already.

An implementation that wants placeholders will have to implement some number of strategies for priority. If they get the number of placeholders they want, cool. If not, then they have to find another strategy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianswett was the main proponent for the "or bust" portion of this recommendation, I believe. Want to defend the sentence?

A PRIORITY frame MAY reference a Placeholder ID that is equal to or greater than
the server's advertised value of the `SETTINGS_NUM_PLACEHOLDERS` settings. When
receiving a PRIORITY frame with its prioritized element set to such a
placeholder, the server SHOULD discard the frame without making any change to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
placeholder, the server SHOULD discard the frame without making any change to
placeholder, the server MAY discard the frame without making any change to

Note that the setting is loosely defined, as is the size of the priority tree. A server might allow placeholders above its limit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer "SHOULD."

Having SHOULD here, along with the recommendation that the server would support at least 32 placeholders (or zero), gives a client the chance to build a prioritization tree that works well.

Changing this to MAY makes that difficult. Consider the case where the client uses a Firefox-like priority tree a server advertises SETTINGS_NUM_PLACEHOLDERS of zero but in fact supports one. Having one placeholder being active while dependencies of other placeholders being assigned to root causes a nightmare.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SHOULD makes no sense. It says that there is an interoperability requirement to put those nodes in the orphan bucket. What does anyone gain from this? The client is prioritizing without regard for server limits, but if the server can build a valid tree, why not let it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This certainly depends on acknowledging that servers can set SETTINGS_NUM_PLACEHOLDERS to zero.

If we look at HTTP/2 implementations, it is my understanding that browsers tend to give higher weight values to resources of higher precedence, regardless of their position in the prioritization tree. This works somewhat reasonably as a fallback when connecting to a server that respects the weights only, because resources of higher precedence would be given more bandwidths that those of lower precedence.

Recommending (or acknowledging) that SETTINGS_NUM_PLACEHOLDERS to be 0 or at least 32 allows a client implementation to consider only two ways of server prioritizing the responses: one being the server respecting the tree build by the client, the other being the tree squeezed and only the weight values being respected.

IMO knowing that the servers are likely to view the prioritization information in either of these two ways is highly preferable for enabling a client to use one design for the prioritization tree it builds regardless of the value of SETTINGS_NUM_PLACEHOLDERS, which is IMO the primary goal of this PR.

receiving a PRIORITY frame with its prioritized element set to such a
placeholder, the server SHOULD discard the frame without making any change to
the priority tree. When receiving a PRIORITY frame with its element dependency
set to such a placeholder, the server SHOULD update the priority tree with the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set to such a placeholder, the server SHOULD update the priority tree with the
set to a discarded placeholder, the server SHOULD update the priority tree with the

Note that this isn't any different than referring to a long-expired request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if "discard" is the correct word, because placeholders are never discarded. They simply exist (or do not).

How about "nonexistent"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

kazuho and others added 2 commits June 26, 2019 13:20
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
@MikeBishop
Copy link
Contributor

I think this PR was on a good track, but given the direction at IETF 105 to remove the priority tree from the document, I'm closing the PR. If we decide to retain the tree-based scheme in H3 (or resurrect it as an extension), I would support incorporating this. Thanks for the work you put into it, @kazuho.

@MikeBishop MikeBishop closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Placeholder limits make for awkward clients
7 participants