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

Add httpbeast #295

Closed
dom96 opened this issue Jul 18, 2018 · 50 comments · Fixed by #1786
Closed

Add httpbeast #295

dom96 opened this issue Jul 18, 2018 · 50 comments · Fixed by #1786

Comments

@dom96
Copy link
Contributor

dom96 commented Jul 18, 2018

https://github.com/TechEmpower/FrameworkBenchmarks/tree/master/frameworks/Nim/httpbeast

@waghanza
Copy link
Collaborator

@dom96 does httpbeast support routing ?

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

Depends how sophisticated the routing has to be. It is pretty barebones though.

@waghanza
Copy link
Collaborator

waghanza commented Jul 18, 2018

@dom96 the routes are pretty simple like described on #221

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

Is doing some string handling acceptable?

if request.path.startsWith("/user/"):
  let id = request.path[6 .. ^1]

@waghanza
Copy link
Collaborator

waghanza commented Jul 18, 2018

@dom96 why not ^^^

Pour purpose is to compare performances, so no matter what impl

@tbrand what do you think about this ?

@OvermindDL1
Copy link
Collaborator

OvermindDL1 commented Jul 18, 2018

why not ^^^

tbrand didn't accept that identical testing in the C++ version though, hence why it was converted to the slower regex path matcher. ;-)

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

But why? :(

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

#181 (comment)

That's a shame :/

@waghanza
Copy link
Collaborator

waghanza commented Jul 18, 2018

Sorry, mis-usage if irony here

For me it is totally inacceptable.

Why ? Because if routing feature as use on Real world applications.

jester and mofuw accepts dynamic routes, but wai (PR wating) is not -> we have to implement wai-routing to merge this PR

I think @tbrand will be on the same page (thats why I ping him)

What do you think about this ?

Sorry again for this

@waghanza
Copy link
Collaborator

For me parsing URL with a regexp is good (I have juste noticed that regexp un evhtp impl is too open).

What we want (for me) is routing, no matter what technic is used (regexp, or elle). However, this step SHOULD to be done as a dispatcher -> I mean handling http req to route to a spécifications code (not handling all urls in one function)

PS : @OvermindDL1 evhtp is enabled, but we have to restrict route

@OvermindDL1
Copy link
Collaborator

PS : @OvermindDL1 evhtp is enabled, but we have to restrict route

It has quite a variety of routing interfaces, pick one? The regex one is the slowest though so if that is what you are wanting to compare...

@waghanza
Copy link
Collaborator

@OvermindDL1 could you list the routing interface ?

In the case of evhtp , routing has to be used only to check /user/%d on GET

@waghanza
Copy link
Collaborator

@dom96 could httpbeast routing be more restrictive ?

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

More restrictive? What do you mean?

You can use regex for it too if you want, but I find that a little silly.

@waghanza
Copy link
Collaborator

I come from ruby world, and it does not blow my eyes when I read a route

get '/user/[\d]+, ....

But perhaps there is a smarter way to do (having one place where is defined callback in use and params), feel free to share ^^

@OvermindDL1
Copy link
Collaborator

@OvermindDL1 could you list the routing interface ?
In the case of evhtp , routing has to be used only to check /user/%d on GET

It has exact matching (exact string match, which most frameworks support), it has prefix matching (like how the elixir and many other frameworks primarily route on), it has regex matching (which frameworks like elixir do not support at all and you have to build it yourself), it has custom matching (supply your own matching and routing handler) that I know of off hand, and I think I'm missing one. In general the order supplied above is the efficiency that they run at (although the custom matching might be better or worse depending on what you do). Full full key processing and validation checking you use the regex matcher (which is what the evhtp benchmark is doing now).

get '/user/[\d]+, ....

That's essentially what evhtp is doing currently (although not \d I think as that wass not part of the readme requirements at the time, is it now?).

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

I come from ruby world, and it does not blow my eyes when I read a route

My suggestion is surely not that much harder to read. The advantage is that it should be many times faster than resorting to Regex.

@waghanza
Copy link
Collaborator

@OvermindDL1 it's quite the case https://github.com/tbrand/which_is_the_fastest/blob/master/cpp/evhtp/main.cpp#L41 but there is not restriction
How about prefix matching ? Could you show me a example

@dom96 does routing could be done something like one one line ?

@waghanza
Copy link
Collaborator

@OvermindDL1 do you have time to implement custom matching

if it's the fastest way to have routing, so why not having this in this project

@OvermindDL1
Copy link
Collaborator

OvermindDL1 commented Jul 18, 2018

@OvermindDL1 it's quite the case /cpp/evhtp/main.cpp@master#L41 but there is not restriction

evhtp_set_regex_cb(htp, "^/user/([^/]+)", on_request_user_index, NULL); is precisely the restriction, it only matches that specific route path, equivalent to match "/user/:blah", CallbackModule, :callbackFunction in Elixir's Phoenix for example (although Elixir's would be faster since that is a prefix matcher).

So what restriction are you speaking of?

@OvermindDL1 do you have time to implement custom matching

That's essentially what I had originally but tbrand didn't like it and deleted ethtp entirely (wtf...) as it wasn't "built in"... >.>

@waghanza
Copy link
Collaborator

@OvermindDL1 I think a mis-understand match "/user/:blah" is more talkative to me, so it might be my confusion 😛

That's essentially what I had originally

Could you then give me an URL (or commit hash) ?

@OvermindDL1
Copy link
Collaborator

OvermindDL1 commented Jul 18, 2018

This is where I did it with a combination prefix matcher and custom parser:
https://github.com/tbrand/which_is_the_fastest/blob/24b8f660bd2cbe1bfa80da391c4ac3d9ed264dd7/cpp/evhtp/main.cpp

Specifically this line:
https://github.com/tbrand/which_is_the_fastest/blob/24b8f660bd2cbe1bfa80da391c4ac3d9ed264dd7/cpp/evhtp/main.cpp#L24

const char *p = req->uri->path->full + 6; // 6 being the length of "/user/"

Being the line he objected to as it was not a built-in parser/matcher, where the regex does (it stores the matched segment into argument storage inefficiently and all just as he wanted).

EDIT: As a note, the above work is the exact same amount of work that a couple other frameworks (a rust one that I copied the handling of) did, so I don't know why it was not accepted nor why tbrand just removed it outright as it seemed exceedingly hypocritical (which was my whole issue with it...).

@OvermindDL1
Copy link
Collaborator

@OvermindDL1 I think a mis-understand match "/user/:blah" is more talkative to me, so it might be my confusion stuck_out_tongue

What that does is the same as the regex, just a different syntax form of it that is more efficient as it does less work but also means the data is not in any expected form (have to validate it yourself and can't change the matcher based on the data type unlike how you can do with regex or a custom matcher).

@dom96
Copy link
Contributor Author

dom96 commented Jul 18, 2018

@dom96 does routing could be done something like one one line ?

I don't want to bloat httpbeast with routing. Plus the code I gave you is only two lines, it's really not that much.

@waghanza
Copy link
Collaborator

@OvermindDL1
Copy link
Collaborator

@OvermindDL1 personally, I prefer the /cpp/evhtp/main.cpp@24b8f66 impl

As do I, but tbrand didn't and he deleted it all entirely outright, all in a single commit with other changes as well, thus preventing a rollback (still very wtfery there). Thus the regex implementation that lost ~10% of the performance overall based on quick tests at the time.

@waghanza
Copy link
Collaborator

@tbrand Is there any warnings about that ?

@dom96
Copy link
Contributor Author

dom96 commented Jul 19, 2018

I'll make a PR if @tbrand gives the OK :)

@waghanza
Copy link
Collaborator

waghanza commented Oct 5, 2018

@tbrand ok ?

@dom96 I'm OK for your PR ;-) (and jester fix if you have time)

@tbrand
Copy link
Collaborator

tbrand commented Oct 10, 2018

Can we say that the below code has a feature of routing?

if request.path.startsWith("/user/"):
  let id = request.path[6 .. ^1]

@dom96 You said

Extremely fast HTTP responses in Nim.

Yeah that's sound great! But the feature does not match to the concept of this repo.
May be @waghanza will create other repos that will bench other features. I hope the httpbeast could be included in it.

@waghanza
Copy link
Collaborator

@tbrand For me httpbeast has routing feature (even if I personally found this not elegant comparing to ruby or crystal, but it's a matter of taste)

for me it's OK to include httpbeast in this repo

@tbrand
Copy link
Collaborator

tbrand commented Oct 10, 2018

@waghanza How?

For me httpbeast has routing feature

This way?
#295 (comment)

@waghanza
Copy link
Collaborator

@tbrand Yep

I totally agree that this writing way COULD be strange for some devs, but this code match routes :

  • /user/0
  • /user/1
  • /user/2

but not /users/1.

For me it is a routing feature in the way that a route is handled by a portion of code

@waghanza
Copy link
Collaborator

@tbrand also httpbeast is on tfb https://github.com/TechEmpower/FrameworkBenchmarks/tree/master/frameworks/Nim
probably it COULD be considered as a framework with the features we expect here

@tbrand
Copy link
Collaborator

tbrand commented Oct 10, 2018

Hmm I don't think so. If we accept it, it means every languages level implementation can be accepted. (e.g. Crystal, Ruby, ...)
But if you wish, you could add it.

@waghanza
Copy link
Collaborator

waghanza commented Oct 10, 2018

you do not seems convinced ...

@OvermindDL1 what do you think ? (I do not want to take decisions unilaterally)

@dom96 for me an important point is : how we do not get out-of-sync ?

@dom96
Copy link
Contributor Author

dom96 commented Oct 10, 2018

every languages level implementation

What does that mean? Does it mean "every HTTP server implementation without a router?"

To be fair, your repo is called "web frameworks" so I can understand if you don't want HttpBeast in here. But IMO you should set the bar higher. Routing is relatively trivial.

for me an important point is : how we do not get out-of-sync ?

What do you mean "out-of-sync"?

@waghanza
Copy link
Collaborator

@dom96 I mean having outdated implementations of each frameworks

@dom96
Copy link
Contributor Author

dom96 commented Oct 10, 2018

It shouldn't be a big deal since you've pinned the versions.

@waghanza
Copy link
Collaborator

@dom96 If I can remind you, it will be better. I mean if codebase has to be updated, I prefer that its you (or an other nim dev) to take this task

@OvermindDL1
Copy link
Collaborator

@OvermindDL1 what do you think ? (I do not want to take decisions unilaterally)

Seems fine to me to include it, what is being tested here isn't even the languages but rather how efficient their socket and string processing capabilities are considering how little time is spent in each language proper. If you really wanted to do testing of the frameworks then you'd need to test a lot more, concurrency, websockets, etc... etc... All that is being tested here is socket handling and some simple string processing.

@waghanza
Copy link
Collaborator

of course @OvermindDL1, a proper way to have a full-feature benchmark is to define a complete set of specs

I can handle work on implementations, but have no time to define a complete set of specs, if you are interested, feel free to write specs, I'll handle the coding part ;-)

@dom96 for me it's OK, you can open a PR

@dom96
Copy link
Contributor Author

dom96 commented Oct 11, 2018

Okay, may take me a bit. Busy with a house move right now :)

@waghanza
Copy link
Collaborator

No hurry 😊

@waghanza
Copy link
Collaborator

@dom96 ping ^^

@dom96
Copy link
Contributor Author

dom96 commented Jul 22, 2019

heh, I still have no time for this, sorry. Happy to review PRs if you want to implement httpbeast benchmarks yourself

@waghanza
Copy link
Collaborator

I'll try

@waghanza
Copy link
Collaborator

@dom96 any time to make a PR ?

@waghanza
Copy link
Collaborator

@dom96 The parsing way is not realy important, the most important here is to have one parsing reflecting the real-word usage

@dom96
Copy link
Contributor Author

dom96 commented Aug 14, 2019

Yeah, sorry, again, I'm happy to review any PRs. It should be fairly simple for you to implement this since you know exactly what you'd like out of the benchmark.

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 a pull request may close this issue.

4 participants