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

Optimize page content decoding #85

Merged
merged 26 commits into from May 15, 2017
Merged

Optimize page content decoding #85

merged 26 commits into from May 15, 2017

Conversation

lexmag
Copy link
Collaborator

@lexmag lexmag commented May 13, 2017

By using single match context, decoding gains ~35% speedup.

@lexmag lexmag requested a review from whatyouhide May 13, 2017 19:37
Copy link
Owner

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 👎 on these style changes personally. I think consistent do/end reads better :bowtie:

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])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lexmag merged commit f3bf609 into master May 15, 2017
@lexmag lexmag deleted the am/optimize-decoding branch May 15, 2017 11:23
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 this pull request may close these issues.

None yet

2 participants