Optimize internals

[kristofka]

Hi I did some work optimizing internals (encoding/decoding). I've removed all the expensive calls to Kernel.apply and used iolists allowing to encode data in a more efficient fashion. It might require a bit more polish but it's promising since we get around 2x improvement for decoding and much more when encoding long lists.

[sourcelevel-bot[bot]]

Hello, @kristofka! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[sourcelevel-bot[bot]]

SourceLevel has finished reviewing this Pull Request and has found:

• 1 possible new issue (including those that may have been commented here).

See more details about this review.

[florinpatrascu]

This is so awesome. Thank you so much for this PR.

[dominique-vassard]

Hi, The performance improvement is welcome, everything that makes bolt_sips faster is great!f

However, I've 2 remarks:

• for maps and lists, I don't see the mechanism keeping the actual bolt version throughout the encoding process. This is useful in order to rely on BoltV1 for the structure in itself, but on Bolt Vx for the data inside the structure. This means that encoding [~D[2000-01-01], "hello"] in Bolt V2 (or V3) should be processed like this:

  [] -> encoded by BoltV1 encoder
  ~D[2000-01-01] -> encoded by BoltV2 encoder
  "hello"  -> encoded by BoltV1 encoder

  without keeping track on the bolt version used in first place, you end up to this:

  [] -> encoded by BoltV1 encoder
  ~D[2000-01-01] -> encoded by BoltV1 encoder
  "hello"  -> encoded by BoltV1 encoder

  and this breaks as dates are not supported in Bolt V1

• the second one is more about readability and maintainability: as I can see, now EncoderVx files are useless (and can be deleted), and all encoding functions are in EncoderHelper. On the readability point of view, if EncoderVx files are deleted, the code is no longer documenting the protocol structure in an easy way. Opening EncoderV2 files makes it easier to know what happened in Bolt V2 than searching for when bolt_version <= 3 and when bolt_version <= 3 and when bolt_version >= 2 and when bolt_version <= 3. Regarding maintainability, when a new bolt version will be out, the guards should be changed. Then, for example: when bolt_version <= 3 becomes when bolt_version <= 4. In my opinion, implementing a new bolt version should not imply updating previous bolt version code. Maybe we could find a middle path between readability and performance. As you mentioned, Kernel.apply is not efficient, so if we can end up with one compiled file composed of multiple other file before compilation, this could be great. The hard part is that all bolt version should be available at runtime.

[dominique-vassard]

I don't know if this could work, but what about something like this?

# Define the functions list
defmodule BoltSips.EncodingFunctionsV3 do
  defmacro __using__(_opts) do
    quote do
      def encode(:atom, data) do
        #Override EncodingFunctionsV2 encoding
      end

    end

    use BoltSips.EncodingFunctionsV1

    def encode(:new_type, data) do
      ...
    end
  end

end

defmodule BoltSips.EncodingFunctionsV2 do
  use BoltSips.EncodingFunctionsV1

  defmacro __using__(_opts) do
    quote do
      def encode(:date, data) do
        ....
      end

    end
  end
end

defmodule BoltSips.EncodingFunctionsV1 do
  defmacro __using__(_opts) do
    quote do
      def encode(:atom, data) do
        ....
      end

      def encode(:list, data) do
        ...
      end
    end
  end
end

# Build fully fleshed encoders with all required functions
# No need to fallback to a previous encoder anymore
defmodule BoltSips.EncoderV1 do
  use BoltSips.EncodingFunctionsV1
end

defmodule BoltSips.EncoderV2 do
  use BoltSips.EncodingFunctionsV2
end

defmodule BoltSips.EncoderV3 do
  use BoltSips.EncodingFunctionsV3
end

# EncoderHelper just needs to dispatch the calls
defmodule BoltSips.EncoderHelper do
  def call_encode(data_type, data, bolt_version) when bolt_version == 3 do
    BoltSips.EncoderV3.encode(data_type, data)
  end

  def call_encode(data_type, data, bolt_version) when bolt_version == 2 do
    BoltSips.EncoderV2.encode(data_type, data)
  end
  def call_encode(data_type, data, bolt_version) do
    BoltSips.EncoderV1.encode(data_type, data)
  end

end

[kristofka]

Regarding readability, I've moved the implementation in macro modules in this branch (see internals/packstream/v1.ex ...) . This gives us both more readable code and the benefits that the functions are called locally at runtime.

Regarding the first issue, I'm not sure I understand :

If the map is encoded with bolt v >=2 then the version will be passed along and dates will properly be encoded with v2 while the map itself will use v1. If the map is encoded with v1 then there shouldn't be date encoding. Am I missing something?

I can make a new pull request with the code moved in macros.

[dominique-vassard]

Regarding readability, I've moved the implementation in macro modules in this branch (see internals/packstream/v1.ex ...) . This gives us both more readable code and the benefits that the functions are called locally at runtime.

That's really nice :+1:

Regarding the first issue, I'm not sure I understand :

If the map is encoded with bolt v >=2 then the version will be passed along and dates will properly be encoded with v2 while the map itself will use v1. If the map is encoded with v1 then there shouldn't be date encoding. Am I missing something?

You're not missing anything, I was stuck in my fallback-to-previous-version mindset and didn't realize that wasn't the approach here. There is no problem, sorry for the misunderstanding.

I can make a new pull request with the code moved in macros.

Sure, thanks. @florinpatrascu will have the final on the PR but I think he will be pleased :)

One last thing about v1.ex and v2.ex would be to replace:

• v1.ex: when bolt_version <= 3 by when bolt_version <= @last_bolt_version
  • v2.ex: when bolt_version >= 2 and bolt_version <= 3 by when bolt_version >= 2 and bolt_version <= @last_bolt_version where @last_bolt_version Bolt.Sips.Internals.BoltVersionHelper.last() Then no need to change anything when the next bolt version will come up, as it is planned for june next year if I don't mistaken, and there seems to be no changes on packstream.

Thanks for your great work. It's nice to have more people contributing to bolt_sips!

[kristofka]

With the latest commits I believe I have addressed most concerns from @dominique-vassard , let me know if you think I should change anything

[florinpatrascu]

Thank you both, for ideas and implementation! This is an awesome contribution. I will try to take a shot at streaming server responses, after I return from vacation, and this new code makes all that work easier. ❤️