Skip to content

Conversation

@p8
Copy link
Contributor

@p8 p8 commented Jan 28, 2025

Return JSON from the routes is a lot faster:

name branch_name json
rails-iodine master 105369
rails-iodine rails/json-from-route 144006

@p8 p8 force-pushed the rails/json-from-route branch from b01d03c to 1de1af2 Compare January 28, 2025 08:27
Return JSON from the routes is a lot faster:

+------------+------------------------------+------+
|        name|                   branch_name|  json|
+------------+------------------------------+------+
|rails-iodine|                        master|105369|
|rails-iodine|         rails/json-from-route|144006|
+------------+------------------------------+------+
@p8 p8 force-pushed the rails/json-from-route branch from 1de1af2 to cec5f91 Compare January 28, 2025 08:32
@rsamoilov
Copy link
Contributor

IMHO, the problem with such changes is that the resulting code is not production-grade.

Nobody uses ActionController::Metal in production, nobody uses lambda handlers in production (maybe except for health routes), and nobody builds header objects manually. This means that the numbers produced by this new test will not be valid, as nobody will ever be able to see them in the real world.

@p8
Copy link
Contributor Author

p8 commented Feb 11, 2025

IMHO, the problem with such changes is that the resulting code is not production-grade.

Thanks for looking at this PR @rsamoilov !

I can see the problem taking things too far. For example, we could implement the whole application in config/routes.rb.
Nobody builds an app like that.
But if I had to create a production grade Rails application that had to be as performant as possible, I'd add changes like this PR.

Also, Rails comes with a lot of functionality out-of-the-box, that the tests don't require and other frameworks don't implement, like CSRF-protection.
While other frameworks, like Roda, starts out basic, extra functionality can be enabled with plugins.
So I think for a Rails application to fall back to more bare metal API's is acceptable.

Nobody uses ActionController::Metal in production

ActionController::Metal is part of the Rails API,
The documentation even has an example that would work for the plaintext test.
So I think it's acceptable to use it.

BTW this change removes usage of ActionController::Metal.

nobody uses lambda handlers in production (maybe except for health routes)

I work on applications that have lambda handlers in routes. For example:

get 'googleabcdef12345.html' => lambda { |env| [200, { }, ['google-site-verification: googleabcdef12345.html'] ] }

For the plaintext implementation we could even move it to public/plaintext.html, but that probably would fail because of the missing headers.

and nobody builds header objects manually.

Some servers, for example puma, don't include the date header, while others like unicorn and iodine include it.
The server header isn't typically set by Rails, so I think it's ok to add.

The Rails documentation also describes setting the headers.

@p8
Copy link
Contributor Author

p8 commented Feb 11, 2025

@rsamoilov Btw, I think it would be totally acceptable to do this for Rage as well.
For example for the plaintext implementation:

Rage.routes.draw do
  root to: ->(env) { [200, {}, "It works!"] }

  get "json", to: "benchmarks#json"
  get "plaintext", ->(env) { [200, {
         'Server' => 'Rails',
         'Content-Type' => 'text/plain',
       },
       ['Hello, World!']]]
  }
  get "db", to: "benchmarks#db"
  get "queries", to: "benchmarks#queries"
  get "fortunes", to: "benchmarks#fortunes"
  get "updates", to: "benchmarks#updates"
end

@rsamoilov
Copy link
Contributor

But if I had to create a production grade Rails application that had to be as performant as possible, I'd add changes like this PR.

Fair enough.

Btw, I think it would be totally acceptable to do this for Rage as well. For example for the plaintext implementation:

I don't think it would make it much faster. With JSON and plaintext routes, most overhead comes from scheduling fibers and parsing parameters - and this code is called for lambda handlers, too.

@p8
Copy link
Contributor Author

p8 commented Feb 11, 2025

Locally I get:

branch plaintext
master 200568
rage/plaintext-from-routes 257473

@rsamoilov
Copy link
Contributor

rage/plaintext-from-routes | 257473

Quite unexpected.

Btw, I think it would be totally acceptable to do this for Rage as well.

I think I would prefer regular controllers in the Rage test - you almost won me over, but I still think it's not a production-grade code. You don't have access to parameters inside lambda handlers, no request/response objects, and no authentication.

The strength of Ruby (and Rails) is its high level of abstraction. Rage proves that this high level of abstraction doesn't have to be slow. But with such low-level lambda-handler code, you might as well use Go - and it will also be faster.

@p8
Copy link
Contributor Author

p8 commented Feb 16, 2025

rage/plaintext-from-routes | 257473

Quite unexpected.

The things is that the json and plaintext calls are micro benchmarks that aren't realistic in production.
They are so minimal that code that is negliglible in a real world application suddenly becomes a bottleneck.
For example for Roda the json call spends 25% of the time just generating the Date header:

image

But these calls still add to the composite score. And if other frameworks have special paths for json and plaintext I think Ruby frameworks should use them as well.
Having Ruby score high is beneficial for the whole Ruby ecosystem I think.

I think I would prefer regular controllers in the Rage test - you almost won me over, but I still think it's not a production-grade code. You don't have access to parameters inside lambda handlers, no request/response objects, and no authentication.

For calls where we do need to access parameters we still use regular controllers.
I think it's fair to bypass parameter parsing or for example CSRF protection if a call doesn't require it.
I hope the tests can be made more strict, for example by validating HTTP compliance. Maybe that would require just using controllers.

The plaintext test requirements state that the test "... is an exercise of the request-routing fundamentals only..".
I think these changes still test routing a request to a response.

The strength of Ruby (and Rails) is its high level of abstraction. Rage proves that this high level of abstraction doesn't have to be slow. But with such low-level lambda-handler code, you might as well use Go - and it will also be faster.

Some of the Ruby frameworks are faster than some of the Go frameworks.
I think each frameworks should be allowed to use all its public API to get the best score.

@rsamoilov
Copy link
Contributor

Well, I think you have a point.

@NateBrady23 NateBrady23 merged commit 8ff228e into TechEmpower:master Mar 3, 2025
3 checks passed
@p8 p8 deleted the rails/json-from-route branch March 3, 2025 18:15
@josevalim
Copy link
Contributor

The plaintext test requirements state that the test "... is an exercise of the request-routing fundamentals only..".
I think these changes still test routing a request to a response.

All benchmarks will, by definition, test routing a request and response. The trouble is that you are fully bypassing Rails' request/response abstractions and benchmarking Rack instead. I think Metal was less problematic because it was still a Rails feature.

You are right that using Rack apps in the router is a Rails feature but then it begs the question of where to draw the line: for example, should frameworks that offer the ability to hijack the socket, use such feature to directly read/write to a socket?

But these calls still add to the composite score. And if other frameworks have special paths for json and plaintext I think Ruby frameworks should use them as well.

Both links are still using the framework request/response abstractions to write their responses instead of sending canned responses as this PR does. I'd say they are much closer to the Metal example that was here before.

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