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

Replace param placeholder from ?<number> to ? #7

Merged
merged 2 commits into from
Aug 9, 2018

Conversation

scarfacedeb
Copy link
Contributor

@scarfacedeb scarfacedeb commented Aug 7, 2018

Performance Issues

We've noticed that ClickhouseEcto added a lot of overhead to insert times (compared to db=<num>ms ecto times)

Profiling revealed that ClickhouseEcto.Connection.order_params/2 was responsible for almost 25-30% of execution time. (Regex.replace calls in particular)

To eliminate the overhead, we've changed param placeholder format from ?1, ?2, ?3 to ?. And removed order_params as a result, because it was no longer needed.

Need for ?1, ?2 params

We haven't noticed any regressions after removing ordered params, but to be sure, I've ported mysql unit tests from ecto.

There's a similar code in mssql ecto adapter that is required for DATEADD mssql functions to work.

But we couldn't find any such cases in clickhouse_ecto, therefore we assumed that it was safe to remove ordered params completely.

Test app

I've created a test app to demonstrate the performance differences: https://github.com/scarfacedeb/clickhouse_ecto_test

A sample of the profiling results

Using profile.fprof with the following command:

mix profile.fprof -e 'ClickhouseEctoTest.test_insert' --callers=true --no-warmup --sort=own

ClickhouseEcto 0.2.4:

14:17:06.277 [debug] QUERY OK db=115.2ms decode=0.3ms queue=0.7ms
INSERT INTO "stats" ("fan","ip","temp","temp2","ts","ts_date","user_id") VALUES (?,?,?,?,?,?,?),...

                                                                   CNT    ACC (ms)    OWN (ms)
Total                                                           107537     669.164     522.200

:re.loopexec/7                                                     732       0.000     170.587
:re.do_replace/5                                                    15       1.400       0.203
Regex.run/3                                                          4       0.091       0.091
Regex.match?/2                                                       1       0.023       0.023
Regex.do_replace/4                                                   2     174.019       0.007
Regex.scan/3                                                         1      13.529       0.004
  :re.run/3                                                        755     189.062     170.915  <--
    :re.loopexec/7                                                  40       0.000       0.250
    :re.grun/3                                                      18     188.734       0.109
    :suspend                                                        40       0.909       0.000

Enumerable.List.slice/3                                          61425       0.000     162.325
Enum.slice_any/3                                                   350     180.195       0.895
  Enumerable.List.slice/3                                        61775     180.195     163.220  <--
    Enumerable.List.slice/3                                      61425       0.000     162.325
    :garbage_collect                                                42      10.990      10.990
    :suspend                                                        19       5.985       0.000

....

Our fork without order_params:

14:56:57.239 [debug] QUERY OK db=86.9ms decode=0.1ms queue=45.5ms
INSERT INTO "stats" ("fan","ip","temp","temp2","ts","ts_date","user_id") VALUES (?,?,?,?,?,?,?)...

                                                                   CNT    ACC (ms)    OWN (ms)
Total                                                            38451     270.859     134.829

URI."-encode_www_form/1-lbc$^0/2-0-"/2                            3212       0.000      16.964
URI.encode_www_form/1                                                4      49.227       0.011
  URI."-encode_www_form/1-lbc$^0/2-0-"/2                          3216      49.227      16.975  <--
    URI."-encode_www_form/1-lbc$^0/2-0-"/2                        3212       0.000      16.964
    URI.percent/2                                                 3212      31.839      15.519
    :garbage_collect                                                10       0.378       0.378
    :suspend                                                         2       0.035       0.000

URI."-encode_www_form/1-lbc$^0/2-0-"/2                            3212      31.839      15.519
  URI.percent/2                                                   3212      31.839      15.519  <--
    anonymous fn/1 in URI.encode_www_form/1                       3212      12.036       6.786
    URI.hex/1                                                     1856       3.202       3.182
    :garbage_collect                                                15       0.954       0.954
    :suspend                                                         5       0.128       0.000

....

@907th
Copy link

907th commented Aug 8, 2018

It needs to be refined that MySQL unit tests can not be used to detect a necessity of param (re)ordering just because these tests include only ?s (not ?1, ?2, ...). But we believe (and tested) that mssql_ecto introduced params reordering to only support DATEADD(...) func.

@santaux
Copy link
Contributor

santaux commented Aug 9, 2018

@scarfacedeb @907th Thank you guys! You are awesome! 👍

@santaux santaux merged commit ff5c8db into clickhouse-elixir:master Aug 9, 2018
@scarfacedeb scarfacedeb deleted the refactor_params branch August 9, 2018 09:20
fermuch pushed a commit to fermuch/clickhouse_ecto that referenced this pull request May 25, 2023
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

3 participants