minor perf: update filter state method to reduce repeated hash searching #19609

wbpcode · 2022-01-19T13:19:28Z

Commit Message: minor perf: update filter state method to reduce repeated hash searching
Additional Description:

Minor optimization by simple code update. The fromFilterState will search FilterState multiple times. Some of these searches are duplicates. Try to reduce some unnecessary overhead by simplifying the api.

Before:

After:

Risk Level: Low.
Testing: N/A.
Docs Changes: N/A.
Release Notes: N/A.
Platform Specific Features: N/A.

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

mattklein123

Thanks for the perf fix. A couple of API questions, thank you.

/wait

mattklein123 · 2022-01-24T19:48:59Z

envoy/stream_info/filter_state.h

+   * An exception will be thrown if the data is not mutable.
+   */
+  virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE;
+
  /**
   * @param data_name the name of the data being probed.
   * @return Whether data of the type and name specified exists in the
   * data store.
   */
  template <typename T> bool hasData(absl::string_view data_name) const {


Is this still used anywhere? Can it be removed to avoid using the pattern you removed in this PR?

Yes, it is still used when we try to set some value to the filter state.

if(!filter_state.hasData<Type>(name)) { filter_state.setData(...); }

Seems like this will also have the same double hash lookup issue? Maybe setData() should have a parameter about whether to set of it already exists? WDYT?

It looks like it's more complicated than I thought it would be. The parent filter state and the mutable flag make it's hard to change the current hasData then setData code pattern.

@htuch Sounds reasonable to reserve two different API (getReadOnly/getMutable). But I am not sure if we should add special flag(StateType) for every filter state object.

I feel less strongly about the state type aspect. It seems like it could catch some kinds of errors, which all things being equal is a good thing. If there really is a genuine performance issue, we can probably eliminate, but I'd vote note to change it unless this is the cause of the observable performance loss.

@htuch StateType do not result in observable performance loss. But it makes the implementation of FilterState complex and there are some unsafe scenarios.

The following code is a common use of FilterState:

if(filter_state.hasData<Type>(name)) { auto state = filter_state.getDataMutable<Type>(...); // ...... }

But hasData will return true if there is a read only filter state object. Then the getDataMutable may throws un-catched exception.

However, the StateType can be used to avoid the FilterState Object being overwritten or modified. 🤔 So, there are both advantages and disadvantages.

The example you give is interesting. I think we should have a pattern such as getDataMutableOrNull and then use that instead of hasData; that seems the real unsafe thing. I'm not going to strongly advocate for this but I do see some advantage and fixing the above pattern in any case seems desirable.

Sounds great. I will try to solve it in a new PR. 🤔

envoy/stream_info/filter_state.h

wbpcode · 2022-02-08T02:47:21Z

@mattklein123 @htuch Thanks for all your comments. Sorry for the delayed replies. I have just finished my long holiday.

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

…r_state_minor_update

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

mattklein123 · 2022-02-08T16:37:58Z

Can you check CI and I will take a look?

/wait

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

…r_state_minor_update

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

wbpcode · 2022-02-09T14:02:52Z

https://storage.googleapis.com/envoy-pr/dc9afac/coverage/index.html

mattklein123

Thanks this LGTM with small comments.

/wait

mattklein123 · 2022-02-09T16:44:49Z

envoy/stream_info/filter_state.h

-    return *result;
+  template <typename T> const T* getDataReadOnly(absl::string_view data_name) const {
+    const auto* generic_result = getDataReadOnlyGeneric(data_name);
+    return generic_result == nullptr ? nullptr : dynamic_cast<const T*>(generic_result);


nit I think you can remove the conditional here and collapse this into a single line. dynamic_cast on nullptr is safe so you can just return that directly. Same below.

mattklein123 · 2022-02-09T16:47:33Z

source/common/router/header_formatter.cc

      return std::string();
    }

+    auto typed_state = dynamic_cast<const StringAccessor*>(state);


Per other comment dynamic_cast on nullptr is OK so you can collapse this and make it simpler. Same elsewhere.

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

mattklein123

Thanks!

…ing (envoyproxy#19609) Signed-off-by: wbpcode <[email protected]> Signed-off-by: Josh Perry <[email protected]>

wbpcode added 3 commits January 18, 2022 07:33

update fromFilterState to reduce hash serching

7adcdf3

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

minor update

bc71b9d

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

minor update & fix format

b8d02f3

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

mattklein123 requested changes Jan 24, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 24, 2022

mattklein123 self-assigned this Jan 24, 2022

code cleanup for filterstate

4addb66

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

wbpcode requested review from alyssawilk, dio, ggreenway, lizan and rojkov as code owners February 8, 2022 06:24

repokitteh-read-only bot removed the waiting label Feb 8, 2022

wbpcode added 2 commits February 8, 2022 08:28

Merge branch 'main' of https://github.com/envoyproxy/envoy into filte…

53767aa

…r_state_minor_update

fix some test

a901b02

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

repokitteh-read-only bot added the waiting label Feb 8, 2022

fix some test

9c62424

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

repokitteh-read-only bot removed the waiting label Feb 9, 2022

wbpcode added 2 commits February 9, 2022 06:20

Merge branch 'main' of https://github.com/envoyproxy/envoy into filte…

8df4b08

…r_state_minor_update

update per file coverage to avoid ci flaky

0e91746

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

mattklein123 requested changes Feb 9, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 9, 2022

minor updates to simplify code

76fa3fd

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

repokitteh-read-only bot removed the waiting label Feb 10, 2022

mattklein123 approved these changes Feb 10, 2022

View reviewed changes

mattklein123 merged commit e74999d into envoyproxy:main Feb 10, 2022

joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022

minor perf: update filter state method to reduce repeated hash search…

99c95bc

…ing (envoyproxy#19609) Signed-off-by: wbpcode <[email protected]> Signed-off-by: Josh Perry <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor perf: update filter state method to reduce repeated hash searching #19609

minor perf: update filter state method to reduce repeated hash searching #19609

wbpcode commented Jan 19, 2022 •

edited

Loading

mattklein123 left a comment

mattklein123 Jan 24, 2022

wbpcode Jan 25, 2022

mattklein123 Jan 25, 2022

wbpcode Jan 26, 2022

wbpcode Jan 26, 2022

wbpcode Feb 8, 2022

htuch Feb 9, 2022

wbpcode Feb 9, 2022

htuch Feb 10, 2022

wbpcode Feb 10, 2022

wbpcode commented Feb 8, 2022

mattklein123 commented Feb 8, 2022

wbpcode commented Feb 9, 2022

mattklein123 left a comment

mattklein123 Feb 9, 2022

wbpcode Feb 10, 2022 •

edited

Loading

mattklein123 Feb 9, 2022

wbpcode Feb 10, 2022

mattklein123 left a comment

minor perf: update filter state method to reduce repeated hash searching #19609

minor perf: update filter state method to reduce repeated hash searching #19609

Conversation

wbpcode commented Jan 19, 2022 • edited Loading

mattklein123 left a 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

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

wbpcode commented Feb 8, 2022

mattklein123 commented Feb 8, 2022

wbpcode commented Feb 9, 2022

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

wbpcode commented Jan 19, 2022 •

edited

Loading

wbpcode Feb 10, 2022 •

edited

Loading