Conversation
|
The following frameworks were updated, pinging maintainers: |
| /// Pre-built complete JSON response (rebuilt once/sec). | ||
| json_response: [u8; 160], | ||
| json_len: usize, |
There was a problem hiding this comment.
I think than this is not permitted.
Could you explain ?
msmith-techempower
left a comment
There was a problem hiding this comment.
The crates directory feels like it should be hosted elsewhere. It looks like it's the actual framework being exercised and we wouldn't expect an end-user (e.g., a developer) to have that included in their project.
|
@msmith-techempower Didn't know that, thanks. Will move it to a separate repo. |
|
@joanhey @msmith-techempower I pushed the fixes |
| // - JSON: high volume | ||
| // - DB/queries/updates JSON: medium volume | ||
| // - Fortunes HTML: lower volume | ||
| for i in 0..500_000u32 { |
There was a problem hiding this comment.
This is very confusing to me - can you explain to me how (and where to look in the code) the json test is implemented? I see the comment // Single JSON response below, but I don't see any allocations.
Also, this loop has me confused as well.
There was a problem hiding this comment.
@msmith-techempower This is just a PGO build tool, not the server. The actual code is in the framework repo here
There was a problem hiding this comment.
I'm still extremely confused. I'm not sure where the disconnect is, but the only code I would expect to see in this pull request to our repo is application code for starting your framework, declaring and implementing your routes and logic, and a Dockerfile and benchmark_config.json.
Basically, if someone said "I'm looking to start a Vortex project" you could point them to this directory and it should be clear what they need to copy/build.
There was a problem hiding this comment.
I see your point. I will make PR cleaner.
| // Realistic fortune data (matches TFB schema) | ||
| let fortunes: Vec<(i32, String)> = vec![ | ||
| (1, "fortune: No such file or directory".into()), | ||
| (2, "A computer scientist is someone who fixes things that aren't broken.".into()), | ||
| (3, "After enough decimal places, nobody gives a damn.".into()), | ||
| (4, "A bad random number generator: 1, 1, 1, 1, 1, 4.33e+67, 1, 1, 1".into()), | ||
| (5, "A computer program does what you tell it to do, not what you want it to do.".into()), | ||
| (6, "Emacs is a nice operating system, but I prefer UNIX. \u{2014} Tom Christensen".into()), | ||
| (7, "Any program that runs right is obsolete.".into()), | ||
| (8, "A list is only as strong as its weakest link. \u{2014} Donald Knuth".into()), | ||
| (9, "Feature: A bug with seniority.".into()), | ||
| (10, "Computers make very fast, very accurate mistakes.".into()), | ||
| (11, "<script>alert(\"This should not be displayed in a browser alert box.\");</script>".into()), | ||
| (12, "\u{30D5}\u{30EC}\u{30FC}\u{30E0}\u{30EF}\u{30FC}\u{30AF}\u{306E}\u{30D9}\u{30F3}\u{30C1}\u{30DE}\u{30FC}\u{30AF}".into()), | ||
| ]; |
There was a problem hiding this comment.
There was a problem hiding this comment.
@joanhey I already replied to @msmith-techempower (see comment above).
There was a problem hiding this comment.
@joanhey @msmith-techempower I removed that file, should be cleaner now. Let me know if anything else looks off.
There was a problem hiding this comment.
I'll ask again.
What is this ?
https://github.com/yp3y5akh0v/vortex/blob/master/techempower/src/profgen.rs#L44-L58
Using an ORM, all Fortune objects must be fetched from the Fortune table, and placed into a list data structure. Tests that do not use an ORM will be classified as "raw" meaning they use the platform's raw database connectivity.
The list data structure must be a dynamic-size or equivalent and should not be dimensioned using foreknowledge of the row-count of the database table.
And precisely the code for the bench need to be in this repo (not in your repo).
There was a problem hiding this comment.
And where is the code that connect to the database? and execute the SQL ?
EDIT: I found the db code.
There was a problem hiding this comment.
That's a build-time PGO profiling tool - it runs once during docker build to generate compiler optimization data, then gets thrown away. The hardcoded fortune strings are just dummy input for the profiler. At runtime, fortunes are fetched from the database on every request, no caching. You can see the server code here: https://github.com/yp3y5akh0v/vortex/blob/master/crates/vortex-server/src/server.rs
There was a problem hiding this comment.
OK.
But use dummy input, not the same from the database and not exactly a 12 list, as the list size and the content is unknown.
|
@joanhey @msmith-techempower Hi, just checking in — all feedback has been addressed. Let me know if anything else is needed. |
|
For me is not OK. |
|
@joanhey @msmith-techempower benchmark code is now in this repo, got it sorted already |
|
@joanhey @msmith-techempower any update on this? Benchmark code is in this PR now as requested, and framework code is in my repo. |
No, it isn't. How do you explain the fact that your benchmark code does not contain the string |
|
@volyrique @joanhey @msmith-techempower I have addressed the issues, should be good now. lmk if all good now with this PR |
frameworks/Rust/vortex/src/app.rs
Outdated
| const PLAINTEXT_CT: &[u8] = b"text/plain"; | ||
| const PLAINTEXT_BODY: &[u8] = b"Hello, World!"; | ||
| const JSON_CT: &[u8] = b"application/json"; | ||
| const JSON_BODY: &[u8] = b"{\"message\":\"Hello, World!\"}"; |
There was a problem hiding this comment.
For each request, an object mapping the key message to Hello, World! must be instantiated.
There was a problem hiding this comment.
@joanhey JSON serialization is now done per request. Please review when you get a chance.
New Rust framework with all endpoints.