search

the-benchmarker / web-frameworks

Which is the fastest web framework?
MIT License
7k stars 672 forks source link

All Crystal frameworks are using a now deprecated function #6443

Open jwoertink opened 1 year ago

jwoertink commented 1 year ago

As of Crystal 1.8, Process.fork is now deprecated https://github.com/crystal-lang/crystal/blob/master/CHANGELOG.md#180-2023-04-14

In src/lucky.cr:13:3

 18 | Process.fork do
              ^---
Warning: Deprecated Process.fork. Fork is no longer supported.

A total of 1 warnings were found.

Here's the PR where it was deprecated.

All of the current Crystal frameworks use these lines in order to spin up multiple processes for speed.

https://github.com/the-benchmarker/web-frameworks/blob/f4fb1bc43de8d4ff0dec3dba79569c5b249f6f4e/crystal/lucky/src/lucky.cr#L12-L13

https://github.com/the-benchmarker/web-frameworks/blob/f4fb1bc43de8d4ff0dec3dba79569c5b249f6f4e/crystal/toro/src/toro.cr#L23-L24

Since Crystal 1.8 deprecates this, and Crystal 1.9 could potentially remove it, we may need to find a new way to test all of these.

waghanza commented 1 year ago

Thanks for catch up @jeremyevans, we definitely need to upgrade codebase.

Could also be great to have some warning on deprecation (I mean using CI or else)

waghanza commented 1 year ago

cc @Blacksmoke16 @stakach @ujjwalguptaofficial @drujensen @Sija

crimson-knight commented 1 year ago

@jwoertink @waghanza I made #6558 to test using this approach. I currently use this for a small binary that needs to trigger multiple child processes.

Unfortunately I couldn't get the test suite to run on my local machine because docker-machine requires VirtualBox which would only install if I wasn't on an M1. I can try playing around with this again later if no one else gets it working.

Blacksmoke16 commented 1 year ago

Could just ignore it, it's only deprecated so not like its going anywhere anytime soon. Maybe by the time Crystal 2.0 comes around, MT support will be better supported and can just leverage that. Might even work already, but haven't did much with it yet. Plus multi process doesn't have the same overhead so iirc gives better perf anyway.

stakach commented 1 year ago

Spider-gazelle already supports non-forked clustering, replacing this line with:

server.cluster(process_count, "-w", "--workers") if process_count != 1

(and threads if built with threading enabled - although then you'd want to disable clustering)

crimson-knight commented 1 year ago

@jwoertink I switched the Amber benchmark to use Process.new and that appears to be working just fine:

https://github.com/the-benchmarker/web-frameworks/blob/master/crystal/amber/src/amber.cr