Improve performance of finding SNI Actions

[masaori335]

Summary

@c-taylor found a performance issue with SNI Action lookup. If we have a lot of configs(FQDNs) in sni.yaml, it spends time on pcre_exec. Because ATS traverses the SNIList sni_action_list.

Benchmark & Flamegraph

My benchmark says 27% drop in throughput (req/sec) with 400 FQDNs in the sni.yaml.

fqdn(s) in sni.yaml	0	1	100	200	400
req/sec	619853.15	610165.85	600232.46	537234.01	453022.71
diff		98.43716	96.83462	86.67118	73.08549

Approach

Add a std::unordered_map for the exact match. This is similar to what we have with certification lookup.
The wildcard cases are left as is in the SNIList sni_action_list for compatibility.

[ywkaras]

Why not call it CharConvert.h?

[masaori335]

How about StingConvert.h ?

[masaori335]

Other candidate is TextConvert.h, but it sounds like utils for the TextView?

[ywkaras]

I don't understand how the flame graph is related to this PR. It might be interesting on it's own, if you could read more of the function names. Is it graphing data from gstack/pstack ?

[masaori335]

I forgot add some context of the flame graph. As you pointed, the call stack doesn't say where the pcre_exec come from (probably due to optimization or I'm missing something with perf record). However, we found huge differences by having many (400) FQDNs in the sni.yaml or not. Also, the pcre_exec disappears by this patch with the same configs.

[SolidWallOfCode]

Is it really faster / simpler to downcase the strings vs. using a case insensitive comparison in the unordered_map?

[masaori335]

I'm following what SSLCertLookup does for consistency. Initially, I used a custom hash and compare function, but I haven't seen significant differences.

[ywkaras]

Is it really faster / simpler to downcase the strings vs. using a case insensitive comparison in the unordered_map?

He would also have to write a custom hash to do the case conversion before accumulating each character. It's also tricky to avoid temporary strings generated from string_view or null terminated strings with maps of strings.

[ywkaras]

I've been kicking around the idea in my head that generating a single DFA to match against a list of strings with wild cards would only be medium hard. It would probably be faster than what we're doing (a DFA per string). Even if the strings had no wildcards, it might be competitive with an unordered_map of the strings.

Another idea is to OR together all the strings into one expression, and see what sort of DFA pcre could generate from that.

[masaori335]

There are a ton of approaches to optimize the lookup function, but we can do it later. This change fixes the 27% drop at least, let's ship this first.

[ywkaras]

I'm hearing that other Yahoo people want to have input on this.

[bneradt]

It sounds like @JosiahWI's patch to add incoming port ranges to sni.yaml may provide a way to address this:
#9767

The suggestion from @maskit (see #9767 (comment)) is that we can add a separate directive for servername's with patterns that pcre would apply to versus plain sni literals. That way the latter would not run through the pcre logic.

[ywkaras]

The detection of wildcards is very simple ( https://github.com/apache/trafficserver/pull/9736/files#diff-abc27229b33119467f5ec2357eac6a114e5e874fe6db8ce5014c3920683c1ac8R119 ) so I'm not understanding why we would need a distinct key for FQDNs with wildcards.

[bneradt]

The detection of wildcards is very simple ( https://github.com/apache/trafficserver/pull/9736/files#diff-abc27229b33119467f5ec2357eac6a114e5e874fe6db8ce5014c3920683c1ac8R119 ) so I'm not understanding why we would need a distinct key for FQDNs with wildcards.

That makes sense. Can you please add your comment to the discussion here:
#9767 (comment)

[ywkaras]

The detection of wildcards is very simple ( https://github.com/apache/trafficserver/pull/9736/files#diff-abc27229b33119467f5ec2357eac6a114e5e874fe6db8ce5014c3920683c1ac8R119 ) so I'm not understanding why we would need a distinct key for FQDNs with wildcards.

That makes sense. Can you please add your comment to the discussion here: #9767 (comment)

Hmm from the code I don't see that a new YAML key is added for FQDNs with wildcards. Only a new key for incoming port range.

[bneradt]

The detection of wildcards is very simple ( https://github.com/apache/trafficserver/pull/9736/files#diff-abc27229b33119467f5ec2357eac6a114e5e874fe6db8ce5014c3920683c1ac8R119 ) so I'm not understanding why we would need a distinct key for FQDNs with wildcards.

That makes sense. Can you please add your comment to the discussion here: #9767 (comment)

Hmm from the code I don't see that a new YAML key is added for FQDNs with wildcards. Only a new key for incoming port range.

@JosiahWI was planning to do that as a separate commit.

[maskit]

I'm not sure if we all are on the same page. The two keys I suggested on #9767 are, one for the standard way, and one for current pcre pattern matching. The both can use * but in different ways.

• A. Standard way
  • 1. No wildcard - Masaori's exact match code works for this case
  • 2. Wildcard ("*" is only allowed at the beginning, and has more restrictions) - PCRE would not be needed
• B. Current way
  • 1. Pattern without "*" - Masaori's exact match code does not always work, because there may be other PCRE stuff
  • 2. Pattern with "*" - Obviously PCRE

If we currently allows use of PCRE, checking * is not enough and it can cause problems if somebody use other pattern matching stuff (i.e. .+, [0-9], etc).

[ywkaras]

We also have the alternative of eliminating regular expressions entirely. Instances of this class encode a pattern with zero or more arbitrarily-located wildcards. (So zero wildcards does not have to be handled as a special case.

#include <string>
#include <string>
#include <vector>

// Each instance of this class is a pattern to be matched against a string_view instance.  A pattern is a non-empty list of
// components.  Each component is a fixed sequence of characters (a segment), or a wild card.  (Neither two successinve wild
// cards, nor two successive fixed sequences, are valid.)  A string_view is a match for a pattern if it can be
// represented as the concatenation of segments, where each segment[n] matches pattern component[n].  A wild card
// matches any segment with zero or more characters.  A fixed component matches if it is equal to the segment.
//
template <typename CharT = char, class Traits = std::char_traits<CharT> >
class Wild_card_pattern
{
public:
  using String_t = std::basic_string<CharT, Traits>;

  using String_view_t = std::basic_string_view<CharT, Traits>;

  // If there are multiple fixed segments, there is an implicit wild card between each of them.
  //
  Wild_card_pattern(bool wild_start, std::vector<String_t> &&fixed, bool wild_end)
  : _wild_start{((fixed.size() == 0) && wild_end) ? true : wild_start}, _wild_end{wild_end}, _fixed{fixed} {}

  bool match(String_view_t sv) const;

private:
  bool const _wild_start, _wild_end;
  std::vector<String_t> const _fixed;
};

template <typename CharT, class Traits>
inline bool Wild_card_pattern<CharT, Traits>::match(Wild_card_pattern<CharT, Traits>::String_view_t sv) const
{
  size_t fixed_idx{0}, sv_pos{0};

  if (_wild_start) {
    if (_fixed.size() == 0) {
      return true;
    }
  } else {
    if (_fixed.size() == 0) {
      return sv.size() == 0;
    }
    if (sv.size() < _fixed[0].size()) {
      return false;
    }
    if (_fixed[0] != sv.substr(0, _fixed[0].size())) {
      return false;
    }
    sv_pos += _fixed[0].size();
    fixed_idx = 1;
  }

  // Try to match all pairs of a wildcard followed by a fixed segment.
  //
  while (fixed_idx < _fixed.size()) {
    size_t ofs = sv.substr(sv_pos).find(_fixed[fixed_idx]);
    if (String_view_t::npos == ofs) {
      return false;
    }
    sv_pos += ofs + _fixed[fixed_idx].size();
    ++fixed_idx;
  }

  return _wild_end || (sv.size() == sv_pos);
}

There are some corner cases to keep in mind. The above code would not consider abab to be a match for *ab. If the call to find() were changed to rfind(), then abab would match *ab, but cabcabc would not match *ab*abc. Not sure what PCRE would do with these cases.

[ywkaras]

Seems like this is turning into a case of the ideal being the enemy of the good. I'll wait till Monday to approve it, to give anyone who wants to a last chance to request changes.

[bneradt]

Seems like this is turning into a case of the ideal being the enemy of the good. I'll wait till Monday to approve it, to give anyone who wants to a last chance to request changes.

There is currently an dev mailing list email discussion about this with subject "[Discussion/Proposal] Regex support in sni.yaml".

[ywkaras]

Seems like this is turning into a case of the ideal being the enemy of the good. I'll wait till Monday to approve it, to give anyone who wants to a last chance to request changes.

There is currently an dev mailing list email discussion about this with subject "[Discussion/Proposal] Regex support in sni.yaml".

Yes, if it (ever) converges on an alternative, we can substitute the alternative for this change.

[masaori335]

The discussion is still going on, but an important comment is "the policy match depends on the order of entries". This PR is missing that point, and we need to support it anyway. I'll follow what remap config does when we reach a consensus.

[masaori335]

Now, this PR has

• using std::unordered_multimap to make SNI Actions lookup faster
• using ats_wildcard_matcher for wildcard support in the fqdn field like SSLCertLookup
• the policy match depends on the order of entries like remap.config