Skip to content

stats: find regex prefixes more aggressively, taking into account ignored dot token#2630

Merged
htuch merged 7 commits intoenvoyproxy:masterfrom
jmarantz:regex-prefix-ignored-dot
Feb 28, 2018
Merged

stats: find regex prefixes more aggressively, taking into account ignored dot token#2630
htuch merged 7 commits intoenvoyproxy:masterfrom
jmarantz:regex-prefix-ignored-dot

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Feb 16, 2018

Description:
This PR expands the scope of regexes where we can parse out a prefix. This has a dramatic effect on the annotated perf information available with #2615 patched in

Before:

Duration(us)  # Calls  Mean(ns)  StdDev(ns)  Min(ns)  Max(ns)  Category  Description                   
     1202203   680147      1767     495.516      474    32449  re-miss   envoy.grpc_bridge_method      
     1177264   680147      1730     1632.82      459   911498  re-miss   cipher_suite                  
     1165345   680147      1713     1057.51      474   754551  re-miss   envoy.grpc_bridge_service     
      689854   680127      1014     1217.74      468   913666  re-miss   envoy.response_code_class     
      677495   680147       996     356.977      474    26636  re-miss   envoy.response_code           
      629426   680147       925     384.233      510    44731  re-miss   envoy.dynamo_partition_id     
      621136   680031       913      317.79      498    25128  re-miss   envoy.http_conn_manager_prefix
      619777   680147       911     323.361      497    27758  re-miss   envoy.dynamo_operation        
      616214   680147       906     324.013      497    31321  re-miss   envoy.mongo_callsite          
      612954   680147       901     350.482      497    37811  re-miss   envoy.dyanmo_table            
      610424   680147       897     322.675      482    29806  re-miss   envoy.http_user_agent         
      605570   680147       890     830.284      476   632688  re-miss   envoy.mongo_collection        
      602257   680147       885     815.602      473   617581  re-miss   envoy.mongo_cmd               
      599676   680147       881     641.186      458   464386  re-miss   envoy.virtual_cluster         
      599098   680147       880     323.564      469    30446  re-miss   envoy.ssl_cipher              
      598951   680147       880     325.497      467    22001  re-miss   envoy.fault_downstream_cluster
      461011   680000       677     419.058      569    29752  re-match  envoy.cluster_name            
          80      116       691      263.19      465     1863  re-match  envoy.http_conn_manager_prefix
          20       20      1032     289.216      601     1430  re-match  envoy.response_code_class     
          15        9      1693     361.928     1395     2608  re-miss   envoy.listener_address        
           4        5       863      381.14      675     1544  re-match  envoy.listener_address        

After:

Duration(us)  # Calls  Mean(ns)  StdDev(ns)  Min(ns)  Max(ns)  Category  Description                   
     1198585   680000      1762     1788.65     1106  1417204  re-miss   cipher_suite                  
     1198240   680000      1762     1109.09     1104   815451  re-miss   envoy.grpc_bridge_method      
     1185443   680000      1743     502.314     1097   110660  re-miss   envoy.grpc_bridge_service     
      671456   680147       987     363.849      494    33352  re-miss   envoy.response_code           
      665013   680127       977     486.461      475    27003  re-miss   envoy.response_code_class     
      625015   680031       919     348.088      577   109116  re-miss   envoy.http_conn_manager_prefix
      459439   680000       675     2302.42      566  1698905  re-match  envoy.cluster_name            
         226      106      2134     919.738      981     7388  re-miss   envoy.dynamo_partition_id     
         213      106      2016     653.666      951     3607  re-miss   envoy.dynamo_operation        
         213      106      2011      828.97      936     7570  re-miss   envoy.dyanmo_table            
         206      106      1947     626.113      907     3528  re-miss   envoy.http_user_agent         
         204      106      1929     623.625      907     3682  re-miss   envoy.fault_downstream_cluster
          81      116       705     293.475      461     2194  re-match  envoy.http_conn_manager_prefix
          34       14      2479     371.675     2010     3099  re-miss   envoy.ssl_cipher              
          21       20      1071     307.876      601     1522  re-match  envoy.response_code_class     
          16        9      1855     365.495     1595     2781  re-miss   envoy.listener_address        
           4        5       870     424.149      645     1627  re-match  envoy.listener_address    

Note that many fewer regexes need to be evaluated, although the really expensive ones are still examined very often. They need to be evaluated less often or made to be cheaper, preferably both.

Risk Level: Low

Release Notes: N/A

Signed-off-by: Joshua Marantz <jmarantz@google.com>
This reverts commit 661911a.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

any chances someone could take a quick look? This has a very large speedup-to-lines-changed ratio

@dnoe dnoe assigned dnoe and htuch Feb 16, 2018
dnoe
dnoe previously approved these changes Feb 16, 2018
Copy link
Contributor

@dnoe dnoe left a comment

Choose a reason for hiding this comment

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

LGTM, @htuch ?

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM

: name_(name), prefix_(std::string(extractRegexPrefix(regex))),
regex_(RegexUtil::parseRegex(regex)) {}

static bool regexStartsWithDot(absl::string_view regex) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you move this to anonymous namespace to match general Envoy style for .cc local definitions?

Copy link
Contributor Author

@jmarantz jmarantz Feb 25, 2018

Choose a reason for hiding this comment

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

I made that exact comment on @gsagula 's gzip filter review and @dnoe commented on my comment:

"I think in most cases of Envoy code base you will see static methods rather than the anonymous namespaces style (I told @gsagula to use the static method style based on that), although there's plenty of anonymous namespace style too. The google style guide is ambivalent about this. Let's standardize on one or the other and codify it in STYLE.md."

Shame on me for not following up. On this particular topic I actually truly don't care -- static vs anon-namespace. Can the two of you decide and I'll write the PR to put it in STYLE.md and (if needed) fix this instance accordingly?

Copy link
Member

Choose a reason for hiding this comment

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

I vote for anonymous namespaces as the standard.

…uire that generally.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

I think all comments are addressed; PTAL.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 2a81a8f into envoyproxy:master Feb 28, 2018
@jmarantz jmarantz deleted the regex-prefix-ignored-dot branch February 28, 2018 21:15
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
* update common file, including lcense

* update makefile override
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action

Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants