filter chain: prefer vector over list for filter chain #37665

wbpcode · 2024-12-15T03:20:47Z

Commit Message: filter chain: prefer vector over list for filter chain
Additional Description:

Use vector to store the filter chain rather than list. Note, we never require to insert/remove element in the intermediate node of filter chain container. In this scenario, vector is much more memory-friend than list.

Risk Level: mid.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

wbpcode · 2024-12-15T03:21:31Z

This is why I created PRs to unify the creation downstream and upstream filter chains. All for this PR.

wbpcode · 2024-12-15T03:24:35Z

hi, @adisuissa , could you take a look at this also if you get some free time? I think you also did some similar work. Thanks.

alyssawilk · 2024-12-16T15:39:55Z

source/common/http/filter_manager.cc

@@ -919,7 +936,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st
        state_.decoder_filter_chain_aborted_ = true;
      }
    }
-    return encoder_filters_.begin();
+    return encoder_filters_.rbegin();


why are we changing these to reverse iterators?
/wait-any

See, the original container of filter chain is list. When a new filter is added, it will be inserted to the begin of the encoder_filters_ to ensure the first added filter will be called at last for encoding path.

And now, we changed the container to vector and the filter will always be appended to the end of the vector. So, we use the reverse iterator to ensure the previous behavior will not be effected.

ah yep, I missed that we inserted in order before and now we don't. IMO it's worth C++ comments either where the vectors are defined or where we get iterators to call out that's what's going on

I think it is commented at the function where the filter is inserted to the container. I will add some more details at there.

alyssawilk · 2024-12-16T16:13:59Z

source/common/http/filter_manager.cc

@@ -42,12 +37,36 @@ void recordLatestDataFilter(const typename FilterList<T>::iterator current_filte
  // correctly iterate over the filters and set latest, but on subsequent onData iterations
  // we'd start from the beginning again, potentially allowing filter N to modify the buffer even
  // though filter M > N was the filter that inserted data into the buffer.
-  if (current_filter != filters.begin() && latest_filter == std::prev(current_filter)->get()) {
-    latest_filter = current_filter->get();
+  if (std::distance(decoder_filters_.begin(), current_filter->entry()) >


ok sorry I feel like I'm live blogging comments but did we need to rewrite this? we've still got iterators so I'd think it would be ok and I find prior syntax easier to read.
/wait-any

I will check this tomorrow to see if it's necessary. I think I rewrite it mainly because I don't want to use a reference to raw pointer. &*

alyssawilk · 2024-12-16T16:14:35Z

source/common/http/filter_manager.cc

@@ -919,7 +936,7 @@ FilterManager::commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_st
        state_.decoder_filter_chain_aborted_ = true;
      }
    }
-    return encoder_filters_.begin();
+    return encoder_filters_.rbegin();


ah yep, I missed that we inserted in order before and now we don't. IMO it's worth C++ comments either where the vectors are defined or where we get iterators to call out that's what's going on

…ector-filter-chain

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

adisuissa

Thanks! I think it makes sense to use a vector instead of a list, as filters aren't removed from the middle of the chain.

One high-level comment about the iteration order:
My personal preference is to have non-simple iterators (those that are changed and picked up in multiple places) use the simple begin()->end() forward iteration. IMHO it makes the code more readable, and one less thing to worry about when this code is modified. From big-O notation reversing the container first should also be an O(N).
Another option is to abstract over the iterators, so it will always look like this is a forward iterator, and the underlying abstraction will take care of the iteration direction.

adisuissa · 2024-12-17T13:11:47Z

source/common/http/filter_manager.cc

-template <class T>
-void recordLatestDataFilter(const typename FilterList<T>::iterator current_filter,
-                            T*& latest_filter, const FilterList<T>& filters) {
+template <class Iterator, class Filter>


Seems that the function's code requires a strict relationship between the types of Iterator and Filter. Can this be rewritten with a single template type?

wbpcode · 2024-12-17T15:50:42Z

My personal preference is to have non-simple iterators (those that are changed and picked up in multiple places) use the simple begin()->end() forward iteration. IMHO it makes the code more readable, and one less thing to worry about when this code is modified. From big-O notation reversing the container first should also be an O(N).
Another option is to abstract over the iterators, so it will always look like this is a forward iterator, and the underlying abstraction will take care of the iteration direction.

Thanks for this comment. It's pretty easy to reverse the vector first because the unique pointer is used. The reason I use the rbegin is because another long-term target is to avoid creating unique pointer of the filter but construct the filter in the vector directly. At that time, the reverse may bring lot of memory copy. (but no sure to that actually).

Another option is to abstract over the iterators, so it will always look like this is a forward iterator, and the underlying abstraction will take care of the iteration direction.

This sound reasonable to me.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

wbpcode · 2024-12-17T16:41:35Z

cc @adisuissa is this what you want? Now, even the PR self is more cleaner.

adisuissa

Thanks, looks better IMHO.
Left a request for adding comments but otherwise LGTM.

source/common/http/filter_manager.h

adisuissa · 2024-12-17T16:54:28Z

source/common/http/filter_manager.h

+  Iterator begin() { return entries.begin(); }
+  Iterator end() { return entries.end(); }
+
+  std::vector<ActiveStreamDecoderFilterPtr> entries;


not sure if the style guide states that it needs to end with _ or not.

it should be added according to our style. added it.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

wbpcode · 2024-12-18T02:28:00Z

I did nothing to quic.

Code coverage for source/common/quic is lower than limit of 93.4 (93.3)

Let'me merge main and try again.

…ector-filter-chain

wbpcode · 2024-12-18T11:53:22Z

retest

wbpcode · 2024-12-18T11:53:28Z

/retest

alyssawilk

much more comfortable with this version, thanks for the update. LGTM modulo two little comment nits
/wait

source/common/http/filter_manager.h

wbpcode · 2024-12-19T02:10:30Z

much more comfortable with this version, thanks for the update. LGTM modulo two little comment nits /wait

That's why need code review! 😄

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

…ector-filter-chain

wbpcode added 2 commits December 14, 2024 15:33

filter chain: prefer vector over list for filter chain

eb353c9

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

fix test

0acf98c

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

wbpcode assigned alyssawilk Dec 15, 2024

wbpcode assigned adisuissa Dec 15, 2024

alyssawilk reviewed Dec 16, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Dec 16, 2024

alyssawilk reviewed Dec 16, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Dec 16, 2024

wbpcode added 2 commits December 17, 2024 03:08

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-v…

9543d68

…ector-filter-chain

address comments

93db941

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

adisuissa reviewed Dec 17, 2024

View reviewed changes

alyssawilk added the waiting label Dec 17, 2024

address suggestion

29f1a52

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

repokitteh-read-only bot removed the waiting label Dec 17, 2024

minor update

695c13c

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

adisuissa reviewed Dec 17, 2024

View reviewed changes

add/move comments and address suggesion

3308613

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-v…

57e405b

…ector-filter-chain

alyssawilk reviewed Dec 18, 2024

View reviewed changes

source/common/http/filter_manager.h Outdated Show resolved Hide resolved

source/common/http/filter_manager.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Dec 18, 2024

wbpcode added 2 commits December 19, 2024 02:34

address comments

e54f546

Signed-off-by: wangbaiping(wbpcode) <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-v…

b2f1a66

…ector-filter-chain

repokitteh-read-only bot removed the waiting label Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter chain: prefer vector over list for filter chain #37665

filter chain: prefer vector over list for filter chain #37665

wbpcode commented Dec 15, 2024

wbpcode commented Dec 15, 2024 •

edited

Loading

wbpcode commented Dec 15, 2024

alyssawilk Dec 16, 2024

wbpcode Dec 16, 2024 •

edited

Loading

alyssawilk Dec 16, 2024

wbpcode Dec 16, 2024

alyssawilk Dec 16, 2024

wbpcode Dec 16, 2024

alyssawilk Dec 16, 2024

adisuissa left a comment

adisuissa Dec 17, 2024

wbpcode Dec 17, 2024

wbpcode commented Dec 17, 2024 •

edited

Loading

wbpcode commented Dec 17, 2024

adisuissa left a comment

adisuissa Dec 17, 2024

wbpcode Dec 17, 2024

wbpcode commented Dec 18, 2024

wbpcode commented Dec 18, 2024

wbpcode commented Dec 18, 2024

alyssawilk left a comment

wbpcode commented Dec 19, 2024

filter chain: prefer vector over list for filter chain #37665

Are you sure you want to change the base?

filter chain: prefer vector over list for filter chain #37665

Conversation

wbpcode commented Dec 15, 2024

wbpcode commented Dec 15, 2024 • edited Loading

wbpcode commented Dec 15, 2024

Choose a reason for hiding this comment

wbpcode Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Dec 17, 2024 • edited Loading

wbpcode commented Dec 17, 2024

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Dec 18, 2024

wbpcode commented Dec 18, 2024

wbpcode commented Dec 18, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

wbpcode commented Dec 19, 2024

wbpcode commented Dec 15, 2024 •

edited

Loading

wbpcode Dec 16, 2024 •

edited

Loading

wbpcode commented Dec 17, 2024 •

edited

Loading