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.

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

Performance issue in mixer Check cache #1531

Closed
skyao opened this Issue Apr 25, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@skyao

skyao commented Apr 25, 2018

hi, I found some performance issue in the code of mixer cache.

In check_cache.cc, about line 100, method Check():

for (const auto &it : referenced_map_) {
    const Referenced &reference = it.second;
    std::string signature;
    if (!reference.Signature(attributes, "", &signature)) {
      continue;
    }
......
}

Here we compute the signature in the for loop? So, if we have many entries in referenced_map, we have to go through and call the signature many times, and almost all of these call return false except last one.

Then in this Signature() method, in referenced.cc, we do two things in this method:

  1. check if match or not
  2. if match, compute the signature

From the logic in Check() method we get that this method should always return false.

So let see the code in Signature() 😀

utils::MD5 hasher;

  for (std::size_t i = 0; i < exact_keys_.size(); ++i) {
    const auto &key = exact_keys_[i];
    const auto it = attributes_map.find(key.name);
    // if an "exact" attribute not present, return false for mismatch.
    if (it == attributes_map.end()) {
      return false;
    }

    hasher.Update(it->first);
    hasher.Update(kDelimiter, kDelimiterLength);

    const Attributes_AttributeValue &value = it->second;
    switch (value.value_case()) {
      case Attributes_AttributeValue::kStringValue:
        hasher.Update(value.string_value());
        break;
utils::MD5 hasher;

  for (std::size_t i = 0; i < exact_keys_.size(); ++i) {
    const auto &key = exact_keys_[i];
    const auto it = attributes_map.find(key.name);
    // if an "exact" attribute not present, return false for mismatch.
    if (it == attributes_map.end()) {
      return false;
    }

    hasher.Update(it->first);
    hasher.Update(kDelimiter, kDelimiterLength);

    const Attributes_AttributeValue &value = it->second;
    switch (value.value_case()) {
      case Attributes_AttributeValue::kStringValue:
        hasher.Update(value.string_value());
        break;

In the implementation, we call the hasher.Update() and check in the same for loop! And the hasher.Update() will finally invoke the openssh/md5.cc to compute the md5, this method is very heavy!

So if this method finally return false, then all of these hasher.Update() will waste our cpu and memory and no benefit. And this hasher.Update() will be invoked many times in Signature() method, also Signature() method maybe be invoked in many times in Check() method. And Check() method will be invoked in every traffic.

I suggest to improve this Signature method, to do the exact key check loop two times:

  1. first loop, just check if match, don't do the hasher.Update()
  2. Only if the result if true in the first loop, we will do the second loop for exact keys to do the hasher.Update and calculate the signature.

If you agree with this improvement, I will submit a PR soon.

@qiwzhang

This comment has been minimized.

Contributor

qiwzhang commented Apr 25, 2018

SGTM. Please go ahead

@skyao

This comment has been minimized.

skyao commented Apr 26, 2018

PR submit, please review.

#1562

It seems a big performance issue, hope we can improve the QPS after it is fixed.

@skyao

This comment has been minimized.

skyao commented Apr 27, 2018

@qiwzhang

I'm sorry that I'm NOT a c++ developer and I don't have development env to complete this PR, especially for the code format. I have check every line by my eyes but it still reports "no formatted"

Can you help to check if this fix is OK and do left code format or more code refactory? You can consider my code as "fake code" and do your fix.

@skyao

This comment has been minimized.

skyao commented Jun 5, 2018

#1751 has merged into master.

Let's close this issue.

@skyao skyao closed this Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment