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.

Sign up for GitHub

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

rewrite buffer implementation to eliminate evbuffer dependency #5441

Merged
merged 48 commits into from
Mar 13, 2019
Merged

rewrite buffer implementation to eliminate evbuffer dependency #5441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
8d8090f
rewrite buffer implementation to eliminate evbuffer dependency
brian-pane Dec 28, 2018
b038d9b
fix clang-tidy warnings
brian-pane Dec 31, 2018
41b9397
fix additional clang-tidy warnings
brian-pane Dec 31, 2018
7845cb6
cleanups based on code review feedback
brian-pane Jan 3, 2019
14722be
cleanups based on code review feedback
brian-pane Jan 4, 2019
7840114
clean up the BufferSlice interface
brian-pane Jan 4, 2019
bfca4f9
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Jan 5, 2019
7094216
formatting fixes and additional comments
brian-pane Jan 5, 2019
fa94625
enable reserve() to use multiple slices
brian-pane Jan 6, 2019
b38b8da
redesign the slice implementation for better performance
brian-pane Jan 9, 2019
33fdb6f
add a faster deque implementation
brian-pane Jan 10, 2019
059c366
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Jan 11, 2019
0af83c1
speed up prepend operations and small reserve/commit operations
brian-pane Jan 11, 2019
5b3f606
add unit tests for Slice and SliceDeque
brian-pane Jan 11, 2019
4f95325
fix comment typos
brian-pane Jan 11, 2019
d4c0340
fix use-after-free error in one of the test cases
brian-pane Jan 11, 2019
14a6513
fix some clang-tidy warnings
brian-pane Jan 11, 2019
068a917
cleanups based on code review feedback, plus more memory-efficient Ow…
brian-pane Jan 15, 2019
d2c27bf
fix an asan error in one of the tests
brian-pane Jan 16, 2019
7d71d0f
support runtime selection of the old or new buffer implementation
brian-pane Jan 23, 2019
feeb6cb
add a command-line flag to enable evbuffer
brian-pane Jan 25, 2019
6705c9a
add a missing initializer
brian-pane Jan 25, 2019
8e5a98d
update tests based on code review feedback, add tests for search meth…
brian-pane Jan 28, 2019
06881ba
cleanups based on code review comments
brian-pane Feb 6, 2019
736f04c
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Feb 6, 2019
125f7b2
switch to new test base class
brian-pane Feb 6, 2019
7640301
fix C++14 non-placement operator delete warning
brian-pane Feb 6, 2019
3bb6e6d
formatting fix
brian-pane Feb 7, 2019
c63e68e
update spelling dictionary, add tests for buffersEqual test utility f…
brian-pane Feb 10, 2019
f0a415d
add asserts to validate subclass casts
brian-pane Feb 10, 2019
6a32042
eliminate the need for const_cast inside the buffer impl
brian-pane Feb 13, 2019
e90174c
cleanups based on code review feedback
brian-pane Feb 16, 2019
81a68e1
use the old buffer implementation by default
brian-pane Feb 20, 2019
56d4988
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Feb 20, 2019
6170ae6
release notes to explain how to enable new buffer impl
brian-pane Feb 21, 2019
be5a550
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Mar 1, 2019
323c39d
restore a new assert lost in the previous merge
brian-pane Mar 1, 2019
e506fd3
fixes related to fuzz tests:
brian-pane Mar 1, 2019
9780a5d
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Mar 3, 2019
cfd9d4a
allow both old and new buffer impls to be used in the same process (t…
brian-pane Mar 4, 2019
3bcd656
formatting fix
brian-pane Mar 4, 2019
d405218
revert changes to metadata_encoder
brian-pane Mar 7, 2019
cf37183
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Mar 7, 2019
c40a58a
changes based on code review feedback:
brian-pane Mar 9, 2019
0d3e1c4
Merge remote-tracking branch 'upstream/master' into buffer
brian-pane Mar 9, 2019
267d269
add runtime overflow checking for buffer length
brian-pane Mar 10, 2019
a9ff465
switch to a compiler-independent detection of buffer length overflow/…
brian-pane Mar 10, 2019
4d552f2
workaround for the coverage build
brian-pane Mar 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Version history
* mysql: added a MySQL proxy filter that is capable of parsing SQL queries over MySQL wire protocol. Refer to ::ref:`MySQL proxy<config_network_filters_mysql_proxy>` for more details.
* http: added :ref:`max request headers size <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.max_request_headers_kb>`. The default behaviour is unchanged.
* http: added modifyDecodingBuffer/modifyEncodingBuffer to allow modifying the buffered request/response data.
* performance: new buffer implementation (disabled by default; to test it, add "--use-libevent-buffers 0" to the command-line arguments when starting Envoy).
Copy link
Contributor

Choose a reason for hiding this comment

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

This may have been addressed earlier in review but I'm going to be lazy here and ask you to catch me up. :-)

Do we have a plan for roll out? Are we submitting this, flipping it and seeing if it sticks? I wish we had a better plan for testing high risk changes than asking Lyft to smoke test, but that has often been what we do with and AFIK they prefer runtime over command line flags (which was part of the impetus behind #6134)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rgs1 would you be interested in testing this with production traffic in your environment? It requires a command-line flag to turn it on.

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 the plan here should be:

  1. Let's ship this PR
  2. Let's set up fuzzing via @htuch. I agree with @htuch that if fuzzing doesn't uncover any issues I will feel pretty confident.
  3. Once any fuzz issues are fixed, let's see if we can get a couple of production users to smoke test (envoy-dev/envoy-users email maybe).
  4. Then remove the CLI flag, flip to default true, and config guard with the new stuff @alyssawilk has in runtime: codifying runtime guarded features #6134

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having finally at least skimmed through this whole PR, I'm not sure using a reloadable flag is appropriate. Many of the functions only work when comparing old buffer to old buffer and new to new, so I think we'd either have to do some plumbing to make sure the state was consistent throughout all buffers on a given connection (which might not even work then given muxed client connections can share a different muxed upstream connection) or we'd have to implement restart flags as well.

Rather one can latch the value of the runtime once where it's currently set, but then it's not actually doing new-style buffers until after a restart. internally we make clear the differences between reloadable and restart flags so you know the difference between when you push things and when they take effect. Alternately we could use the new config guard code it'd just be fairly misleading and we'd want to communicate it doesn't work as advertised in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR used to latch the value just once per process, except for a test override method. But the problem with that was that it broke the coverage test, which expected to be able to concatenate together a lot of tests that originally were designed to be separate programs with their own control over the choice of buffer implementation.

* http: added encodeComplete/decodeComplete. These are invoked at the end of the stream, after all data has been encoded/decoded respectively. Default implementation is a no-op.
* redis: added :ref:`hashtagging <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings.enable_hashtagging>` to guarantee a given key's upstream.
* redis: added :ref:`latency stats <config_network_filters_redis_proxy_per_command_stats>` for commands.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/server/options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,11 @@ class Options {
*/
virtual bool mutexTracingEnabled() const PURE;

/**
* @return whether to use the old libevent evbuffer-based Buffer implementation.
*/
virtual bool libeventBufferEnabled() const PURE;

/**
* @return bool indicating whether cpuset size should determine the number of worker threads.
*/
Expand Down
Loading