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

220: Improve Rails perf 5-10x #335

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

simpl1g
Copy link
Contributor

@simpl1g simpl1g commented Nov 3, 2024

No description provided.

Copy link
Owner

@antonputra antonputra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Can you sum up where 5-10x performance coming from?

@simpl1g
Copy link
Contributor Author

simpl1g commented Nov 3, 2024

Yes,

  • it uses lightweight Fibers instead of Threads to handle concurrency, so it allows more db operations per process
  • Removed unused middlewares
  • Removed prometheus http collector, it adds around 20% and not used in node, send a patch to the prom client with improvement Use binary search for histogram buckets prometheus/client_ruby#316, I guess it can be improved further

Final results for create endpoint, it is not 10x, only 3x RPS and 8x better latency

before:
wrk -c 100 -t 2 -d 10 --latency -s ruby-app/wrk.lua http://localhost:8080/api/devices
Running 10s test @ http://localhost:8080/api/devices
  2 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   163.18ms  205.32ms   1.09s    80.02%
    Req/Sec     0.90k   210.50     1.21k    77.00%
  Latency Distribution
     50%    7.90ms
     75%  314.85ms
     90%  474.04ms
     99%  809.52ms
  17954 requests in 10.01s, 11.95MB read
Requests/sec:   1793.34
Transfer/sec:      1.19MB

after:
wrk -c 100 -t 2 -d 10 --latency -s wrk.lua http://localhost:8080/api/devices
Running 10s test @ http://localhost:8080/api/devices
  2 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    20.20ms   11.63ms 137.45ms   91.93%
    Req/Sec     2.66k   647.28     3.45k    86.00%
  Latency Distribution
     50%   17.30ms
     75%   20.61ms
     90%   26.89ms
     99%   76.76ms
  52996 requests in 10.04s, 16.88MB read
Requests/sec:   5277.25
Transfer/sec:      1.68MB

@antonputra
Copy link
Owner

Thanks again, @simpl1g! I'll compare it with Django in a week or so; Node.js doesn't make sense.

@antonputra antonputra merged commit 288cb25 into antonputra:main Nov 3, 2024
Comment on lines +7 to +9
Device.new(id: 1, uuid: "9add349c-c35c-4d32-ab0f-53da1ba40a2a", mac: "5F-33-CC-1F-43-82", firmware: "2.1.6", created_at: "2024-5-28T15:21:51.137Z", updated_at: "2024-5-28T15:21:51.137Z").attributes,
Device.new(id: 2, uuid: "d2293412-36eb-46e7-9231-af7e9249fffe", mac: "E7-34-96-33-0C-4C", firmware: "1.0.3", created_at: "2024-01-28T15:20:51.137Z", updated_at: "2024-01-28T15:20:51.137Z").attributes,
Device.new(id: 3, uuid: "eee58ca8-ca51-47a5-ab48-163fd0e44b77", mac: "68-93-9B-B5-33-B9", firmware: "4.3.1", created_at: "2024-8-28T15:18:21.137Z", updated_at: "2024-8-28T15:18:21.137Z").attributes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonputra @simpl1g what is the practical meaning of this benchmark?
for example, in node.js just creates hashes, we can do the same in ruby, and we don't need ActiveRecord objects, hash is much lighter and faster

devices = [
  { id: 1, uuid: "9add349c-c35c-4d32-ab0f-53da1ba40a2a", mac: "5F-33-CC-1F-43-82", firmware: "2.1.6", created_at: "2024-5-28T15:21:51.137Z", updated_at: "2024-5-28T15:21:51.137Z"}, 
  { id: 2, uuid: "d2293412-36eb-46e7-9231-af7e9249fffe", mac: "E7-34-96-33-0C-4C", firmware: "1.0.3", created_at: "2024-01-28T15:20:51.137Z", updated_at: "2024-01-28T15:20:51.137Z" },
  { id: 3, uuid: "eee58ca8-ca51-47a5-ab48-163fd0e44b77", mac: "68-93-9B-B5-33-B9", firmware: "4.3.1", created_at: "2024-8-28T15:18:21.137Z", updated_at: "2024-8-28T15:18:21.137Z" },
]

I think it's more correct to load records from postgresql here, it is close to real use

build_response(Device.find(1,2,3), 200)

Copy link
Contributor Author

@simpl1g simpl1g Nov 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is just CPU bound benchmark, to test how well it handles serialisation. 2nd test is for IO.
But I agree that comparing objects with plain hash is not ideal in this case, that's why I added pure ruby example

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ermolaev well i'm going to test Rails with Django, and pure Ruby implementation from this PR with Node.js - #330

def update
if @device.update(device_params)
render json: @device
if device.save
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when rails saving, query wrapped in a transaction, so here we have 3 queries to database

  TRANSACTION (1.2ms)  BEGIN
  Device Create (1.7ms)  INSERT INTO "devices" ("uuid", "mac", "firmware", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5) RETURNING "id"  [["uuid", nil], ["mac", "dfdf"], ["firmware", nil], ["created_at", "2024-11-06 00:53:09.435080"], ["updated_at", "2024-11-06 00:53:09.435080"]]
  TRANSACTION (0.6ms)  COMMIT

this code is sensitive latency to database

I do not know how other frameworks (django,laravel) works, but they can only insertion (1 query)

we can write device.send :create_or_update for skip transaction
it improves performance Requests/sec: 6866.0961 -> 9428.1055 on my mac m1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simpl1g @antonputra any ideas about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ermolaev yeah, it makes sense to disable transactions if Django also is not using them. It is better to check Django before I guess. But you can at least create PR

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it in place for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants