10x speedup for cowboy_rest and dependents (eg cowboy_static) #305

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

dvv commented Nov 1, 2012

httpd_util:rfc1123/1 is way slow. by introducing a faster ad hoc alternative a huge speedup is achieved.
please, consider applying.
tia, --vladimir

Owner

essen commented Nov 1, 2012

Yes, httpd_util dependency is to be removed, there's a ticket for that.

However, why do you replace rfc1123 by rfc2109?

Also io_lib:format is slow too.

Contributor

narma commented Nov 1, 2012

httpd_util:rfc1123_date converts local time to universal, it's for compliance with HTTP/1.1 RFC2616 wich says:
"All HTTP date/time stamps MUST be represented in Greenwich Mean Time
(GMT), without exception. For the purposes of HTTP, GMT is exactly
equal to UTC (Coordinated Universal Time)."

3> cowboy_clock:rfc2109_fast({{2012,11,01},{17,0,0}}).
<<"Thu, 01 Nov 2012 17:00:00 GMT">>
4> httpd_util:rfc1123_date({{2012,11,01},{17,0,0}}).
"Thu, 01 Nov 2012 10:00:00 GMT"`

hence, after the patch time returning in rest handler's, last_modified and expires, is not turning from local to UTC.

So, additional changes required in cowboy_static.erl for this patch:
from
Fileinfo = file:read_file_info(Filepath1),
to
Fileinfo = file:read_file_info(Filepath1, [{time, universal}]),

I don't presume to judge which one is preferable and more reasonable.

Contributor

dvv commented Nov 1, 2012

right, in my barebone static handler i use {time, universal}, so should imo do cowboy_static.
update: here is the crap\c\c\c\ccode

Contributor

dvv commented Nov 1, 2012

@essen i just added _fast suffix to what cowboy_clock exposed. haven't studied diff between both RFCs.

Owner

essen commented Nov 1, 2012

Ultimately we only want to deal with universal time, so this is definitely in the right direction, however the current rfc2109 function should be converted to this, it's only doing local time at the moment because cookies needed it, but cookies are a temporary implementation so the local code can be moved in the cookie module without any issue for now.

Contributor

si14 commented Dec 2, 2012

Can I suggest our lib for time formatting/parsing? It's quite battle tested and fast as hell: https://github.com/selectel/tempo/

Owner

essen commented Dec 2, 2012

No NIFs, sorry.

Owner

essen commented Dec 2, 2012

By the way this depends on #282. First fix the localtime that should always be UTC, then we can fix the parsing here properly.

Contributor

si14 commented Dec 3, 2012

Why don't use such small&pure NIFs like that one?

Owner

essen commented Dec 3, 2012

Because they mess up with the reduction counts, may crash, are harder to upgrade properly, etc. It's also not going to be faster than code written specifically for a datetime format, which only uses a single binary construct.

Owner

essen commented Dec 3, 2012

Done in 8bc6bde. Using the existing rfc1123 code from Cowboy. Dependency on httpd_util is gone!

Thanks for the initial patch and report!

@essen essen closed this Dec 3, 2012

Owner

essen commented Dec 3, 2012

Oh and this didn't need to depend on #282. I was just misled. :)

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