Added DB endpoints to crystal-raw#2894
Added DB endpoints to crystal-raw#2894NateBrady23 merged 4 commits intoTechEmpower:masterfrom foliot:master
Conversation
|
Thanks @foliot for contributing to The Framework Benchmarks! @saturday06, @msmith-techempower, @will, @zane-techempower, @rdp, @jhass, @RX14, @faustinoaq and @ashawnbandy-te-tfb, code you've worked on has been modified. If you have the chance, please review. If you wish to unsubscribe from these notices, please open a Pull Request with the commit message |
foliot
changed the title
frameworks/Crystal/crystal/server.cr
Outdated
| response.status_code = 200 | ||
| response.headers["Content-Type"] = "application/json" | ||
|
|
||
| results = (1..sanitized_query_count(request)).map do |
There was a problem hiding this comment.
You could use JSON.build to stream the array here without allocating, which would be faster.
frameworks/Crystal/crystal/server.cr
Outdated
| } | ||
|
|
||
| data.push(additional_fortune) | ||
| data.sort_by! { |fortune| fortune[:message] } |
There was a problem hiding this comment.
sort_by! is slower than using sort! { |f1, f2| f1[:message] <=> f2[:message] } because sort_by! tries to call the block only once per item (it allocates an entirely new array!). Since f1[:message] essentially has zero cost it's faster to do it like this.
@asterite This seems like a wart in the stdlib, thoughts?
frameworks/Crystal/crystal/server.cr
Outdated
| response.headers["Content-Type"] = "application/json" | ||
|
|
||
| updated = (1..sanitized_query_count(request)).map do | ||
| set_world({id: random_world[:id], randomNumber: rand(1..ID_MAXIMUM)}) |
There was a problem hiding this comment.
ditto on the JSON optimization
|
@RX14 I made the changes you suggested and they unexpectedly didn't seem to improve the results. I'm currently rerunning the original code to double-check, but here is the updated code along with the original and updated results: https://gist.github.com/foliot/5dbae98909896e4f7c391ca24dcd4c59. The arrays may be small enough that the amount of time to manipulate them is negligible compared to querying the DB and writing to IO. In that case, I think the original code is better, since it's less verbose. |
|
@RX14 I ran the tests again and there was some improvement, but there seems to be some variation between tests, so it's hard to say for sure. The code should be more efficient using your suggestions, so we might as well go with that. |
frameworks/Crystal/crystal/server.cr
Outdated
| JSON.build(response) do |json| | ||
| json.array do | ||
| (1..sanitized_query_count(request)).each do | ||
| world = random_world |
There was a problem hiding this comment.
Simply call random_world.to_json(json) and it'll work, and be much cleaner. See here.
frameworks/Crystal/crystal/server.cr
Outdated
| random_world | ||
| JSON.build(response) do |json| | ||
| json.array do | ||
| (1..sanitized_query_count(request)).each do |
There was a problem hiding this comment.
1.upto(sanitized_query_count(request)) do?
| JSON.build(response) do |json| | ||
| json.array do | ||
| (1..sanitized_query_count(request)).each do | ||
| world = set_world({id: random_world[:id], randomNumber: rand(1..ID_MAXIMUM)}) |
There was a problem hiding this comment.
ditto with the to_json but it'll be cleaner to use
world = set_world({id: random_world[:id], randomNumber: rand(1..ID_MAXIMUM)})
world.to_json(json)here.
|
@foliot @RX14 Let me know when you guys want to merge this in. Looks like you're doing iterative work on it, which is great. We won't be releasing a Preview 2 until just after 4th of July so you have some time. Also, while you're in this space, if there is a canonical way to do caching in Crystal it would be great to see another caching test, as we only have a few implementations right now. Test #7 |
|
@nbrady-techempower I saw you implemented that test but crystal doesn't have a canonical way to do it. Personally i'd use redis but that's really not a obvious choice like crystal-db is right now (crystal-db is written by the core team and all the ORMs use it). I wouldn't be comfortable calling it "crystal" without qualifying it (calling it crystal-redis-caching or something) such that other caching methods (in-memory) could be used too. |
|
@nbrady-techempower I'm ready to merge unless @RX14 has some more suggestions. I think we'll have to do some more thinking about how to implement the caching test, since there's not a canonical way of doing that right now. |
4 participants
I adapted the DB endpoints from the Crystal/Kemal framework code.