-
Notifications
You must be signed in to change notification settings - Fork 131
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
haywire will now keep allocating memory as required, as it's not guar… #103
Conversation
d233d18
to
a6e23bd
Compare
I've benchmarked this on my high performance 10GbE environment. Master
This PR
|
@@ -5,8 +5,7 @@ include("common.cmake") | |||
# Haywire | |||
# ---------------------------------------- | |||
project(haywire C) | |||
set(CMAKE_BUILD_TYPE RelWithDebInfo) | |||
#set(CMAKE_BUILD_TYPE Debug) | |||
set(CMAKE_BUILD_TYPE Release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we don't want debug information? I don't really know a lot about these flags. This could be a great idea, please educate me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the difference when it comes to compilation flags when using Release
and `RelWithDebInfo``:
//Flags used by the compiler during release builds.
CMAKE_C_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
//Flags used by the compiler during release builds with debug info.
CMAKE_C_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
We're already setting -O3
, so the only other difference is the usage of -g
, which enables debug symbols to be embedded in the executable. It won't change the code paths, but it will make the executable a bit larger and may or may not cause additional page faults. I guess that's not a big deal given that Haywire is relatively small though.
I'm happy to revert the change if you prefer to have debug symbols by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah lets keep release with debug info. I talked to a bunch of people to just gauge what is common and it seems the sentiment is that if there is a crash having a stack trace is valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will get this one reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned from @hyc that debug info is in its own segment of the file and never gets paged in during a regular run.
I've reviewed this enough to be comfortable and this is a really important PR so 👍 on merging this after making the discussed changes to When you're done, feel free to merge! I'm excited about this change :) When you have the diagrams complete let me know. I'll find a good spot for them in the repo :) |
156865a
to
15e601c
Compare
…anteed that we'll get one and only one fully formed request on each combination of alloc/read calls. Previously, we were assuming that the alloc callback was only called once per request and that the read callback was called when all the data was available. Unfortunately, that's not the case, and the read callback may be called when only a fraction of the incoming data is available, especially under high concurrency. Buffers are now managed in http_request_buffers.*. We use a mark/sweep technique to get rid of requests previously handled by a connection whenever a new buffer chunk is fully processed. This allows us to keep a relatively small buffer. Also, it's now possible to register interest in a given memory region by placing a pin on it and retrieving its location once the request has been fully read in. We will now also deal with bad requests that were causing crashes before, requests that are too long to be processed and that could cause us to run out of memory. Unknown errors are also dealt with now, by returning an appropriate error code. Removed redundant header imports.
15e601c
to
1ff747b
Compare
Merging then. I've added documentation on how buffers are handled in https://github.com/haywire/haywire/blob/1ff747bbe00a69d7c206764299824c5e51d0b52f/docs/buffers.md. |
haywire will now keep allocating memory as required, as it's not guar…
…anteed that we'll get one and only one fully formed request on each combination of alloc/read calls.
Previously, we were assuming that the alloc callback was only called once per request and that the read callback was called when all the data was available.
Unfortunately, that's not the case, and the read callback may be called when only a fraction of the incoming data is available, especially under high concurrency.
Buffers are now managed in http_request_buffers.*. We use a mark/sweep technique to get rid of requests previously handled by a connection whenever a new buffer chunk is fully processed. This allows us to keep a relatively small buffer. Also, it's now possible to register interest in a given memory region by placing a pin on it and retrieving its location once the request has been fully read in.
We will now also deal with bad requests that were causing crashes before, requests that are too long to be processed and that could cause us to run out of memory. Unknown errors are also dealt with now, by returning an appropriate error code.
Removed redundant header imports.
Makes #93 redundant.
Benchmarks
My tests were run on two m4.xlarge machines on AWS with 4 cores each, with the server being run with:
./build/hello_world --threads 4
. The benchmark was run using wrk:./wrk -c 300 -t 300 -d 1m -s pipelined_get.lua --latency http://172.31.5.65:8000 -- 64
.Results
master
this change
cc'ing @jpz @botdes @violetta-baeva