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

Switch HTTP client to Finch #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hkrutzer
Copy link
Contributor

As discussed in #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.

@sofakingworld
Copy link
Member

@hkrutzer Hi!

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

@sofakingworld
Copy link
Member

@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

@hkrutzer
Copy link
Contributor Author

hkrutzer commented Mar 3, 2023

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

@sofakingworld
Copy link
Member

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

@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

@hkrutzer
Copy link
Contributor Author

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?

@sofakingworld
Copy link
Member

sofakingworld commented Mar 15, 2023

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 sofakingworld force-pushed the master branch 4 times, most recently from 87d3426 to e30960d Compare April 27, 2023 22:06
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.

2 participants