pillar icon indicating copy to clipboard operation
pillar copied to clipboard

Switch HTTP client to Finch

Open hkrutzer opened this issue 3 years ago • 7 comments

As discussed in https://github.com/balance-platform/pillar/issues/39, this will allow for the addition of streaming queries. If this PR is accepted I will add those as well.

Some async functions no longer make sense to include so I have deprecated them.

Benchmarking with Benchee shows significant improvement, likely at least partially due to Finch using keepalive.

Benchmarks

defmodule PillarWorker do
  use Pillar,
    connection_strings: ["http://localhost:8123/default"],
    name: __MODULE__,
    pool_size: 10
end
PillarWorker.start_link()
Benchee.run(
  %{
    "standard pool" => fn ->
      conn = Pillar.Connection.new("http://localhost:8123/default")
      sql = "select number FROM numbers(5);"
      {:ok, result} = Pillar.query(conn, sql)
      result
    end,
    "new pool" => fn ->
      sql = "select number FROM numbers(5);"
      {:ok, result} = PillarWorker.query(sql)
      result
    end
  }
)

All benchmarks were performed on the following configuration:

Operating System: macOS
CPU Information: Apple M1 Pro
Number of Available Cores: 10
Available memory: 32 GB
Elixir 1.14.2
Erlang 25.2.3

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 5 s
memory time: 0 ns
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 14 s

Except the ones where I set parallel: 4. Clickhouse was running on the same machine. To get more reliable measurements the benchmark should probably performed on different machines.

With Tesla

Name                    ips        average  deviation         median         99th %
new pool             164.07        6.09 ms   ±140.07%        2.63 ms       43.75 ms
standard pool        163.79        6.11 ms   ±147.86%        2.54 ms       48.27 ms

Comparison:
new pool             164.07
standard pool        163.79 - 1.00x slower +0.0107 ms

With parallel: 4 in Benchee

Name                    ips        average  deviation         median         99th %
standard pool         40.20       24.88 ms   ±159.55%       11.22 ms      206.68 ms
new pool              38.39       26.05 ms   ±141.01%       12.31 ms      189.07 ms

Comparison:
standard pool         40.20
new pool              38.39 - 1.05x slower +1.18 ms

With Finch (this PR)

Name                    ips        average  deviation         median         99th %
standard pool        3.47 K      288.06 μs   ±131.76%      246.13 μs     1086.10 μs
new pool             3.39 K      295.34 μs   ±109.34%      245.67 μs     1276.24 μs

Comparison:
standard pool        3.47 K
new pool             3.39 K - 1.03x slower +7.28 μs

With parallel: 4 in Benchee

Name                    ips        average  deviation         median         99th %
new pool             2.36 K      423.49 μs    ±62.57%      399.38 μs      840.48 μs
standard pool        2.22 K      450.93 μs    ±83.81%      412.25 μs     1166.78 μs

Comparison:
new pool             2.36 K
standard pool        2.22 K - 1.06x slower +27.44 μs

As indicated by these benchmarks, performance can be improved 20 times by using Finch connection pools. Complexity is also reduced because Poolboy is no longer necessary.

Let me know what you think and if you need any changes made to this PR.

hkrutzer avatar Feb 28 '23 10:02 hkrutzer

@hkrutzer Hi!

Thanks for contribution, I'll look ASAP and give feedback

sofakingworld avatar Mar 02 '23 09:03 sofakingworld

@hkrutzer Should I do some extra actions?

I've tried this PR on my project and got error on startup:

Generated XXX app
** (ArgumentError) unknown registry: PillarFinchPool
    (elixir 1.13.0) lib/registry.ex:1343: Registry.key_info!/1
    (elixir 1.13.0) lib/registry.ex:576: Registry.lookup/2
    (finch 0.14.0) lib/finch/pool_manager.ex:44: Finch.PoolManager.lookup_pool/2
    (finch 0.14.0) lib/finch/pool_manager.ex:34: Finch.PoolManager.get_pool/2
    (finch 0.14.0) lib/finch.ex:284: Finch.__stream__/5
    (finch 0.14.0) lib/finch.ex:324: anonymous fn/4 in Finch.request/3
    (telemetry 1.2.1) /home/shpagin/Dev/data_hunter/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (pillar 0.34.1) lib/pillar/http_client.ex:16: Pillar.HttpClient.post/3

sofakingworld avatar Mar 03 '23 14:03 sofakingworld

That pool should be started from Pillar.Application, could there be a reason for that application being started in your project?

hkrutzer avatar Mar 03 '23 16:03 hkrutzer

Do you have production project with pillar library? Could you check with update, how does it work?

In my case I've added as dependency and started tests, 10 tests failed, and by some reason some tests stucked. So mix test never ends

sofakingworld avatar Mar 05 '23 13:03 sofakingworld

@hkrutzer Could you make Finch client as additional? (via config mechanism https://github.com/balance-platform/pillar#http-adapters )

It could fix problem with backward compatibility

sofakingworld avatar Mar 05 '23 14:03 sofakingworld

I can take a look @sofakingworld but I'm not sure it will be possible. In my PR, Poolboy is replaced with Finch. I am not sure it can be done in a way where only other adapters use Poolboy and it is not used with Finch.

Which backward compatibility do you mean?

hkrutzer avatar Mar 14 '23 09:03 hkrutzer

Which backward compatibility do you mean?

To start operation, now you need to start Pillar.Application. And for some reason, tests freeze in the production project, on which I check all the edits.

For this reason, it is necessary that the Finch client be optional and configurable via config

sofakingworld avatar Mar 15 '23 21:03 sofakingworld