Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upOptimize page content decoding #85
Conversation
lexmag
added some commits
May 9, 2017
lexmag
requested a review
from
whatyouhide
May 13, 2017
whatyouhide
reviewed
May 14, 2017
Left a few comments but looks good overall. I'd like to discuss the format of the macros introduced though. I think reading
decode_string(buffer, x)and then using x after reads very confusing because at a glance it's impossible to tell that decode_string/2 is a macro. What if we try out alternate styles that scream "weird macro" in your face a bit more? Some thoughts:
decode_string(x <- buffer)
decode_string_and_bind(buffer -> x)or something weird like that. Wdyt?
| @@ -1,6 +1,26 @@ | ||
| defmodule Xandra.Protocol do | ||
| @moduledoc false | ||
| defmacrop decode_string(buffer, value) do |
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 14, 2017
Collaborator
We need a comment explaining what these two macros do and why we need them.
| @@ -407,7 +427,8 @@ defmodule Xandra.Protocol do | ||
| end | ||
| defp decode_error_message(_reason, buffer) do | ||
| {message, _buffer} = decode_string(buffer) | ||
| decode_string(buffer, message) | ||
| _ = buffer |
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 14, 2017
Collaborator
I think we can improve this as it's a bit "hacky" to do this to ignore unused variable warnings (I assume that's why we do it). For example, we could have decode_string(buffer, message, reuse_buffer: false) or something along these lines. Thoughts?
| end | ||
| end | ||
| defmacrop decode_value(buffer, value, type, do: block) do |
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 14, 2017
Collaborator
Out of curiosity, could you measure the impact of replacing the "do" block here with a function?
decode_value(buffer, value, type, fn value -> ... end)? There is a lot of magic going on here with the magic value variable being assigned before the block and so on. If the impact is negligible, I'd personally go with fn.
| defp decode_value(buffer, -1, _type) do | ||
| {nil, buffer} | ||
| end | ||
| defp decode_value(<<value::bits>>, :ascii), do: value |
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 14, 2017
Collaborator
I am do/end reads better 
lexmag
added some commits
May 14, 2017
| decode_string_list(buffer, size - 1, [elem | acc]) | ||
| defp decode_string_list(<<buffer::bits>>, count, result) do | ||
| decode_string(item <- buffer) | ||
| decode_string_list(buffer, count - 1, [item | result]) |
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 14, 2017
Collaborator
I personally am not in favor of renaming acc to result. result does not say substantially more than acc, and acc is very standard in Erlang/Elixir. I would really push for making this change in another PR with a separate discussion if you really want to do it.
(same for a couple other instances)
This comment has been minimized.
This comment has been minimized.
lexmag
May 15, 2017
Member
Yeah, that acc –> result change is here for a discussion. I started to use result because acc feels even more generic, also it is always an issue when you have more than one accumulator. In this case result means the result of list decoding, seems better to me than just some accumulator.
This comment has been minimized.
This comment has been minimized.
whatyouhide
May 15, 2017
•
Collaborator
Can we have this discussion in another PR though and revert the change here? I don't agree for now and would need more convincing but it would be nice to merge this one soon.
lexmag
force-pushed the
am/optimize-decoding
branch
from
df25b14
to
013743b
May 15, 2017
lexmag
force-pushed the
am/optimize-decoding
branch
from
013743b
to
59c67fc
May 15, 2017
lexmag commentedMay 13, 2017
•
edited
By using single match context, decoding gains ~35% speedup.