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

[fuzz] Made health check fuzz more efficient #13747

Merged
merged 4 commits into from
Oct 29, 2020

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Oct 24, 2020

Signed-off-by: Zach Reyes zasweq@google.com

Commit Message: Made health check fuzz more efficient.
Additional Description: Orthogonal with the same logic as my host/mentor Asra's PR #13729. Took away test session vector for gRPC health checking, as it only ever used one. Also made mock timers in TCP and gRPC NiceMocks to take away uninteresting call logs, which were slowing down the health check fuzzer. Speeds up from 5 -> 30 exec/sec locally.
Risk Level: Low

Signed-off-by: Zach <zasweq@google.com>
@zasweq
Copy link
Contributor Author

zasweq commented Oct 24, 2020

/assign @asraa @adisuissa @htuch

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks!
/wait

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

General question about the memory allocations throughout the code:
Is there any code that handles the deallocation, or is it just deallocated when the test finishes?
In a unit test setting, the tests are somewhat short so it might be ok, but for fuzz tests if this is called multiple times and it could use too much memory.
What do you think?

expectClientCreate(0);
}

void GrpcHealthCheckFuzz::expectClientCreate(size_t index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the given index argument always 0?
If that's the case should we keep this argument, or just remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Asra mentioned that as well :). Removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In regards to your deallocation question, there is no explicit code that deallocates memory. I don't think the memory scales with each fuzz iteration though, as it gets destructed each time.

Signed-off-by: Zach <zasweq@google.com>
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 but did you consider any options for reducing the amount of boilerplate copy+paste here? No need for gymnastics, but if it's possible it would be nice.

test/common/upstream/health_check_fuzz.h Outdated Show resolved Hide resolved
Signed-off-by: Zach <zasweq@google.com>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! Interested to merge and see speedups.
/wait

test/common/upstream/health_check_fuzz.h Outdated Show resolved Hide resolved
Signed-off-by: Zach <zasweq@google.com>
@asraa asraa merged commit 793407b into envoyproxy:master Oct 29, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 30, 2020
* master: (83 commits)
  tls: Typesafe tls slots (envoyproxy#13789)
  docs(example): Correct URL for caching example page (envoyproxy#13810)
  [fuzz] Made health check fuzz more efficient (envoyproxy#13747)
  rtds: properly scope rtds stats (envoyproxy#13764)
  http: fixing a bug with IPv6 hosts (envoyproxy#13798)
  connection: Remember transport socket read resumption requests and replay them when re-enabling read. (envoyproxy#13772)
  network: adding some accessors for ALPN work. (envoyproxy#13785)
  docs: added a step about how to handle platform specific extensions (envoyproxy#13759)
  Fix identation in ip transparency code snippet (envoyproxy#13743)
  wasm: enable WAVM's stack unwinding feature (envoyproxy#13792)
  log: set route name for direct response (envoyproxy#13683)
  Use nghttp2 as external dependsncy in protocol_constraints_lib (envoyproxy#13763)
  [Windows] Update windows dev docs (envoyproxy#13741)
  cel: patch thread safety issue (envoyproxy#13739)
  Windows: Fix ssl_socket_test (envoyproxy#13264)
  apple dns: add fake api test suite (envoyproxy#13780)
  overload: scale selected timers in response to load (envoyproxy#13475)
  examples: Add dynamic configuration (control plane) sandbox (envoyproxy#13746)
  Removed exception in getResponseStatus() (envoyproxy#13314)
  network: add timeout for transport connect (envoyproxy#13610)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.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