Skip to content
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

feat: Simple perf tool for new API #4590

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

purplefox
Copy link
Contributor

@purplefox purplefox commented Feb 19, 2020

Description

Implements #4279

This is a stacked PR so please just review commits from "perf runner" and later

This PR introduces a simple perf tool for rough perf testing of our new API.

It's a simple tool that does not pretend to be perfect or a general purpose performance tool, but it's really useful for quickly running some tests in your IDE to get a rough idea of the overhead of the new API machinery. This can be used as first pass for tuning the server.

There's a base class called PerfRunner where all the magic happens. Then you subclass this for your particular test.

The PR also contains some changes to various buffer sizes and the like, tuned by running this perf tool.

Testing done

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@purplefox purplefox force-pushed the perf_runner branch 2 times, most recently from 5f24f27 to 873117c Compare February 19, 2020 09:45
@purplefox purplefox marked this pull request as ready for review February 19, 2020 11:32
@purplefox purplefox requested a review from a team as a code owner February 19, 2020 11:32
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Hey @purplefox

Why are we not just using JMH here? Feels like we're reinventing the wheel...

Hopefully, in the future, we'll have our existing JHM tests hooked up so that major degredations in performance fail the build. It would be nice if we got these tests also failing the build without any additional work.

protected WebClient client;
protected Server server;

protected volatile Throwable throwable;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: private? There's a protected setter method...

protected void endRun() throws Exception {
sendStream.pause();

Thread.sleep(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sleep with a magic number of ms? Such sleeps in tests are a code smell and often result in intermittent failures.

protected void endRun() throws Exception {
pullQueryEndpoints.closePublishers();

Thread.sleep(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, why the sleep with a magic number of ms? Such sleeps in tests are a code smell and often result in intermittent failures.

@purplefox
Copy link
Contributor Author

Why are we not just using JMH here? Feels like we're reinventing the wheel...

JMH is designed for microbenchmarking of short lived invocations, not really suitable for what we're doing here

@purplefox purplefox merged commit ee7ca8a into confluentinc:master Feb 19, 2020
@purplefox purplefox deleted the perf_runner branch April 28, 2020 07:20
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.

2 participants