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

Improves the performance of getHandle() when routing requests. #484

Closed
wants to merge 1 commit into from

Conversation

atheriel
Copy link
Contributor

This PR contains performance tweaks to the internal getHandle() function used inside of the router, which in turn calls the canServe() method for each handler until it finds one that can take the request. Since this function is essentially "overhead" on every request, it is ideal if it is fast.

The PR makes the following changes:

  • Replaces the use of stri_match() with stri_match_first_regex() directly, removing the expensive and unnecessary calls to match.arg().

  • Imports stri_match_first_regex() into the environment to remove the need to call ::, which was taking up about 25% of the execution time.

  • Takes advantage of the fact that seq can be NULL in for loops to simplify the getHandle() function.

This reduces the overhead of getHandle() by about 50%. From my own testing, this function moved from about 20% of program execution time down to 12% for simple endpoints in our production APIs; for more complex endpoints it moved down from 4% to 2%.

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

This is not a public-facing change, so I don't think it needs an entry in the NEWS.md file, but I can add one if you disagree. Nor does it really require new tests -- existing integration tests should cover these changes.

- Replaces the use of stri_match() with stri_match_first_regex()
  directly, removing the expensive and unnecessary calls to match.arg().

- Imports stri_match_first_regex() into the environment to remove the
  need to call `::`, which was taking up about 25% of the execution
  time.

- Takes advantage of the fact that `seq` can be NULL in for loops to
  simplify the getHandle() function.

This reduces the overhead of getHandle() by about 50%.
@CLAassistant
Copy link

CLAassistant commented Sep 4, 2019

CLA assistant check
All committers have signed the CLA.

@antoine-sachet
Copy link
Contributor

Not that my opinion on this matters much, but this is definitely NEWS-worthy. This is what the "improvements" section is for! Like you, I work on production APIs and a 10% speed-up for simple endpoints is great.

Also I would say the change is public-facing insofar as there will be a measurable effect on all existing plumber APIs.

@schloerke
Copy link
Collaborator

Do you mind sharing how you loadtest or determine the performance increase? Thank you!

@atheriel
Copy link
Contributor Author

atheriel commented Oct 4, 2019

Sure, although I think how I actually did it might be a bit more involved than actually necessary to show the difference:

  • I generated artificial (although realistic) heavy load with wrk. For the test against our production code I ran the API on one server and wrk on another server, but I have run similar tests fully locally.

  • To sample stack traces under this load I used Rprof() (and probably my own external Rprof tool as well, can't quite remember).

  • I used a homebrew R script (which you can find in the linked repo above) to aggregate the Rprof() output and Brendon Gregg's flamegraph.pl to visualize it. I can't share the original visualizations because they contain potentially sensitive function names, but you can get an idea of what they looked like by checking out an issue I filed against httpuv.

@schloerke
Copy link
Collaborator

Thank you. I should be able to repeat something similar on my end.

@atheriel
Copy link
Contributor Author

atheriel commented Oct 4, 2019

A much simpler way, now that I think about it, is just to test the throughput of a sample API before and after the change.

@schloerke
Copy link
Collaborator

Correct. The before and after check works for validating the speed up.

Sounds like you use something similar to profvis for inspecting slow code sections.

—————

@blairj09 and I have been looking into future integrations for plumber to make sure the codebase doesn’t get slower with a particular pr. Currently we have been looking at ab due to ease of use / availability. Also looking into the loadtest R package.

@atheriel
Copy link
Contributor Author

atheriel commented Oct 4, 2019

I'm (obviously) pretty interested in performance regressions as well, and would be happy to see that kind of work.

@meztez
Copy link
Collaborator

meztez commented May 6, 2020

I would still keep the check on NULL handlers in the route function. Checking for null handlers takes about nothing; looping on a NULL takes more. Performance are increase on valid call but you would decrease performance on NULL handlers. Still great catch.

> a <- expression(for (h in NULL) {}); b <- expression(if (!is.null(NULL)) {})
> microbenchmark::microbenchmark(a, b)
# Unit: nanoseconds
#  expr min lq  mean median uq  max neval
#    a   0  0 26.11      1  1 2553   100
#    b   0  0  0.47      0  1    1   100

@schloerke schloerke added the theme:performance:racing_car: I feel the need... the need for speed! label Jun 8, 2020
@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jun 8, 2020
@cpsievert
Copy link
Contributor

The stringi change makes sense, but I don't follow the logic behind the change in getHandle() (how is seq() involved)? Also, I don't see such drastic differences in the minimal example in #484 (comment)

@meztez
Copy link
Collaborator

meztez commented Jun 8, 2020

The stringi change makes sense, but I don't follow the logic behind the change in getHandle() (how is seq() involved)? Also, I don't see such drastic differences in the minimal example in #484 (comment)

It means it is valid to loop on a NULL value in R. seq is in reference to the basic for loop structure for (i in seq). But as shown above, this should be avoided as it takes longer than checking for a NULL value directly because it still has to initialize the loop structure.

tldr : getHandle does not need to be changed.

R/plumber.R Show resolved Hide resolved
schloerke added a commit that referenced this pull request Jun 11, 2020
Co-authored-by: Aaron Jacobs <aaron.jacobs@crescendotechnology.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:performance:racing_car: I feel the need... the need for speed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants