Updated to the latest Octane.#2817
Conversation
|
Thanks @simongui for contributing to The Framework Benchmarks! @nbrady-techempower, code you've worked on has been modified. If you have the chance, please review. If you wish to unsubscribe from these notices, please open a Pull Request with the commit message |
| sds response_buffer = sdsnew("HTTP/1.1 200 OK\r\n"); | ||
| response_buffer = sdscat(response_buffer, "Server: octane\r\n"); | ||
| response_buffer = sdscat(response_buffer, "Content-Type: application/json\r\n"); | ||
| response_buffer = sdscat(response_buffer, "Content-Length: 28\r\n"); |
There was a problem hiding this comment.
Shouldn't this be Content-Length: 27?
{"message":"Hello, World!"} has a length of 27.
There was a problem hiding this comment.
Also, it's a bit 'gamey' to just throw the length in like this instead of computing the length. Just informed that this computation would be at compile-time anyway.
|
@msmith-techempower Fixed, sorry that was left over by accident, no gaming intentions. |
| write_batch* batch = create_write_batch(number_of_requests); | ||
|
|
||
| for (int i=0; i<number_of_requests; i++) { | ||
| if (requests[i]->path[1] == 'p') { |
There was a problem hiding this comment.
Wouldn't this match any path beginning with "/p" (e.g. "/parrot", not just "/plaintext")?
| for (int i=0; i<number_of_requests; i++) { | ||
| if (requests[i]->path[1] == 'p') { | ||
| create_plaintext_response_sds(batch); | ||
| } else if (requests[i]->path[1] == 'j') { |
There was a problem hiding this comment.
I think this is really nit picking because not all uses of HTTP libraries require routing and some HTTP libraries shouldn't dictate how routing syntax behaves either.
Also the benchmark doesn't even stress routing, for example I could implement a common radix trie just to satisfy this request but the code is essentially at runtime going to do the exact same thing as this code. Let's be honest, everyone's routing library in this benchmark is comparing one character or multi-character single instruction comparisons and then routing because the benchmark tests have extremely primitive routes.
So I'll implement this only to satisfy something that takes several hundreds of lines of code that essentially executes a single character comparison because there's no routes that share the same prefix.
This code does not give me any special performance over anyone else because everyones is doing the same thing in the end.
There's no requirements in these benchmarks about requiring a routing module and what features that routing module needs to pass.
There was a problem hiding this comment.
I'll agree with @simongui with "everyone else is doing it" however, I will point out that someday (maybe soon) we may have another p* route, or j* route that's part of the test suite and part of the verification may be testing that those routes exist.
That being said, I'm fine with this for now. @simongui does this give you any performance gains over requests[i]->path = 'plaintext'? Just curious.
I'm sorry if it seems like we're picking on you. It's not the intention. The benchmarks constantly need to improve and evolve and I totally appreciate your discussion in all of it.
There was a problem hiding this comment.
@nbrady-techempower No worries, I'm under the microscope now haha :)
No, it doesn't give me any optimizations for setting the path. Octane uses a lot of zero-copy optimizations (like many of the top results in the benchmarks) but that's totally valid. All that means is Octane reduces the amount of memory copying it's doing and reuses the original buffer used for receiving from the socket by just pointing to positions in the buffer.
I'll probably add more extensive routing at some point but I think it's going to be an optional feature because different people have different routing requirements. I'm trying to build something that has layers where people can opt in to what they want or replace what they don't want.
I actually think a benchmark with a few hundred routes would be a good addition. I suspect various frameworks have much different performance characteristics based on their routing performance that doesn't show up in the plaintext and json benchmarks.
Frameworks who don't have routing would be excluded from that list.
There was a problem hiding this comment.
Agreed, a routing test with a large routing map is in the pipeline.
There was a problem hiding this comment.
Awesome. Hint: wrk supports Lua scripting so you can probably build a routing table in Lua and randomly select from it. I've done similar things for testing HTTP key/value stores with Wrk.
There was a problem hiding this comment.
Yeah we currently use lua for some wrk formatting. However, the routing map will probably be at the python level of the toolset. Finishing up the specs for a cached db test and then I feel like a strong routing test should be next. I'll ping you for input when that happens.
Thanks for your help and patience!
There was a problem hiding this comment.
No problem :) I'm looking forward to getting this merged and a R15 preview run so that I can see where I sit performance wise so I can continue working on things since I kind of rushed this implementation to satisfy the requirements to get out of stripped status haha.
Thanks for handling everything so well!
|
@msmith-techempower this LGTM |
|
Awesome thanks so much :) Is there a PR required to remove this from |
|
Nope, that would be in the |
|
Great, thanks! |
|
@simongui Just wanted to give you a heads up. Octane is hanging in our Server Central environment right after |
5 participants
This PR updates the FrameworkBenchmark code with the latest Octane support. It adds the following after this discussion.
/plaintextand/jsonendpoints.I expect Octane to be removed from the
strippedstatus the discussion above placed on it since I've implemented what was discussed and it can return to being displayed normally along with the other results instead of being hidden.