http: setting default lazy headermap threshold to 3 by adisuissa · Pull Request #16670 · envoyproxy/envoy

adisuissa · 2021-05-26T04:04:36Z

Commit Message: setting default lazy headermap threshold to 3
Additional Description:
PR #12656 introduced the envoy.http.headermap.lazy_map_min_size configuration parameter, that sets the threshold value for the lazy headermap - the number of headers in the headermap after which the headermap will use an underlying dictionary to map the headers.
This PR sets the default value to 3, which ensures that the headermap operations have a linear time-complexity (see #13333).

Risk Level: Medium - sets a different default threshold value.
Testing: None, tests already exist.
Docs Changes: None.
Release Notes: Added note.
Platform Specific Features: N/A.
Fixes #13333

Signed-off-by: Adi Suissa-Peleg adip@google.com

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

snowp · 2021-05-26T13:15:12Z

Can we add a test that demonstrates the new default? Thanks

cc @mattklein123 @alyssawilk in case there are thoughts on what the default should be

alyssawilk · 2021-05-26T13:30:46Z

  ``envoy.reloadable_features.send_strict_1xx_and_204_response_headers``
  (do not send 1xx or 204 responses with these headers). Both are true by default.
 * http: serve HEAD requests from cache.
+* http: set the default lazy headermap threshold to 3, which defines the minimal number of headers in a


Thanks for improving the defaults - I like removing O(n) problems from the default deployment :-)

I think it's not clear from this what the user-visible change is likely to be. I assume there's going to be some user-visible change or we'd just have done a runtime guard of "use_efficient_header_map" and planned to deprecate the old code.

Is it the case that if there's more than 3 non-inline headers, that Envoy will elide multiple header keys into a single entry?

+1 on making this more clear, and potentially even adding a section on this to the HTTP arch overview?

Stepping back, I feel like if we are going to keep this configurability it should probably be a config option or a CLI option, but that would be a larger change. At minimum I would recommend more documentation.

Nice, can you ref link to the new section you added in the arch overview docs? (You will need to add an anchor above that new section.)

sorry I guess I'm still not clear on the user-visible change
Does this just add an extra data structure for fast look-up, but ordering and inlining will remain the same? If so why aren't we just turning this on by default, seeing if anyone complains, and removing the old option? Basically any request and response headers are going to go above 3, so this seems like a lot of complexity for a feature that'd always be on unless someone turns it off to make their own operations more expensive? :-P

I tend to agree that if the actual number we are picking is 3 (as opposed to 10 for example), having it always on and hard coded internally is probably OK. It wouldn't turn on for many trailers cases but that is a minor point.

ah, trailers is a good counterpoint - often only one or two of those. might be worth commenting somewhere the value was selected to result in doing fast mapping for the one and not the other. I wonder if this catches most 100-continue or if they pass through as well. either way feel free to merge once CI passes.

mattklein123

Awesome, thanks for following up on this.

/wait

mattklein123 · 2021-05-26T14:00:58Z

  ``envoy.reloadable_features.send_strict_1xx_and_204_response_headers``
  (do not send 1xx or 204 responses with these headers). Both are true by default.
 * http: serve HEAD requests from cache.
+* http: set the default lazy headermap threshold to 3, which defines the minimal number of headers in a


+1 on making this more clear, and potentially even adding a section on this to the HTTP arch overview?

Stepping back, I feel like if we are going to keep this configurability it should probably be a config option or a CLI option, but that would be a larger change. At minimum I would recommend more documentation.

github-actions · 2021-06-25T16:01:27Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-07-02T20:01:15Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mattklein123 · 2021-07-07T14:43:56Z

@adisuissa can we land this? Can you merge main?

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123

Awesome, thanks. Per offline discussion let's get this in right after we cut the release just so we don't do something high risk right before.

/wait

mattklein123 · 2021-07-13T22:26:53Z

  ``envoy.reloadable_features.send_strict_1xx_and_204_response_headers``
  (do not send 1xx or 204 responses with these headers). Both are true by default.
 * http: serve HEAD requests from cache.
+* http: set the default lazy headermap threshold to 3, which defines the minimal number of headers in a


Nice, can you ref link to the new section you added in the arch overview docs? (You will need to add an anchor above that new section.)

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123 · 2021-07-14T15:05:50Z

Thanks, can you merge main and we can get this in?

/wait

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123

Thank you!

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

http: setting default lazy headermap threshold to 3

24a616b

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

phlax assigned snowp May 26, 2021

alyssawilk self-assigned this May 26, 2021

alyssawilk reviewed May 26, 2021

View reviewed changes

alyssawilk added the waiting label May 26, 2021

mattklein123 requested changes May 26, 2021

View reviewed changes

mattklein123 self-assigned this May 26, 2021

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 25, 2021

github-actions Bot closed this Jul 2, 2021

mattklein123 reopened this Jul 7, 2021

mattklein123 added the no stalebot Disables stalebot from closing an issue label Jul 7, 2021

github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 8, 2021

adisuissa added 3 commits July 13, 2021 14:58

Adding more docs

6aa6a87

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into default_headermap_t…

73b22e5

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into default_headermap_t…

ebd17ae

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only Bot removed the waiting label Jul 13, 2021

mattklein123 requested changes Jul 13, 2021

View reviewed changes

repokitteh-read-only Bot added the waiting label Jul 13, 2021

adisuissa added 2 commits July 13, 2021 20:29

Adding tag

a17e81e

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into default_headermap_t…

d0a0fb7

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only Bot removed the waiting label Jul 14, 2021

repokitteh-read-only Bot added the waiting label Jul 14, 2021

adisuissa added 2 commits July 14, 2021 11:46

Merge remote-tracking branch 'upstream/main' into default_headermap_t…

671b1a1

…hreshold Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Removing non-needed entries after merging new release

5802ac3

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only Bot removed the waiting label Jul 14, 2021

mattklein123 approved these changes Jul 14, 2021

View reviewed changes

mattklein123 merged commit 2f54f36 into envoyproxy:main Jul 14, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

http: setting default lazy headermap threshold to 3 (envoyproxy#16670)

afbd117

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Conversation

adisuissa commented May 26, 2021 • edited by mattklein123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp commented May 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 25, 2021

Uh oh!

github-actions Bot commented Jul 2, 2021

Uh oh!

mattklein123 commented Jul 7, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 14, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adisuissa commented May 26, 2021 •

edited by mattklein123

Loading