erpc-io / eRPC

Efficient RPCs for datacenter networks
https://erpc.io/
Other
835 stars 137 forks source link

Latency calculation error #107

Closed Jerry-Tianchen closed 6 months ago

Jerry-Tianchen commented 7 months ago

Hi"

I realize that there is an error in the calculation of the latency. Apparently, you did not reset the HDR when trying different block sizes....

It will result in large block sizes also have latency like 2.7 us. Please add hdr_reset(c.latency_hist_); under line 195 for app/latency.cc

    if (c.latency_samples_ == c.latency_samples_prev_) {
      printf("No new responses in %.2f seconds\n", kAppEvLoopMs / 1000.0);
      fprintf(stderr, "No new responses in %.2f seconds\n",
              kAppEvLoopMs / 1000.0);
      fflush(stderr);
    } else {
      printf(
          "%zu %.1f %.1f %.1f %.1f %.1f %.1f %.1f %.1f "
          "[%zu new samples, %zu total samples, %zu seconds]\n",
          c.req_size_,
          hdr_value_at_percentile(c.latency_hist_, 50.0) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 5.0) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 99) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 99.9) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 99.99) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 99.999) / kAppLatFac,
          hdr_value_at_percentile(c.latency_hist_, 99.9999) / kAppLatFac,
          hdr_max(c.latency_hist_) / kAppLatFac,
          c.latency_samples_ - c.latency_samples_prev_, c.latency_samples_,
          i / 1000);

          hdr_reset(c.latency_hist_);
    }
    fflush(stdout);
anujkaliaiitd commented 7 months ago

You're right, there should be a hdr_reset() before changing the message size. But line 195 might not be the right place for this, since we also want to track longer-term percentiles beyond one measurement interval. So it needs to be coupled with setting of double_req_size_.

There are a few other boundary issues with the measurement (e.g., req size increases when some smaller old requests are in-flight) too. I've mostly been using this benchmark with the runtime msg size changes disabled.

Jerry-Tianchen commented 7 months ago

I see~ I will also take a look into the issue you say. update you later xD

Thanks Jerry