Bug fix 73 perfromance

[dominique-vassard]

Remove Code.ensure_loaded which was called too many times and causes severe performance drops! Logs slows the query too. A performance has been added too in the test suite. Fix the issue #73

[florinpatrascu]

Hi Dominique - this is awesome, I should've catch that ensure_loaded :( I'd like to merge but the current merge is killing the travis pipeline, plus the logs are in debug mode. Can we turn that off, please? Here's what's failing on travis, in case you can't see in travis directly:

644     test/bolt_sips/performance_test.exs:4
645     Assertion with < failed
646     code:  assert Enum.at(output.scenarios(), 0).run_time_data().statistics().average() < 50000000
647     left:  66795583.45333333
648     right: 50000000
649     stacktrace:
650       test/bolt_sips/performance_test.exs:33: (test)
651
652     The following output was logged:
653     2019-10-09 16:23:19.882 [debug] C: RUN ~ ["CREATE (:Test {value: 'test_1'})\nCREATE (:Test {value: 'test_2'})\nCREATE (:Test {value: 'test_3'})\nCREATE (:Test {value: 'test_4'})\nCREATE (:Test {value: 'test_5'})\nCREATE (:Test {value: 'test_6'})\nCREATE (:Test {value: 'test_7'})\nCREATE (:Test {value: 'test_8'})\nCREATE (:Test {value: 'test_9'})\nCREATE (:Test {value: 'test_10'})\nCREATE (:Test {value: 'test_11'})\nCREATE (:Test {value: 'test_12'})\nCREATE (:Test {value: 'test_13'})\nCREATE (:Test {value: 'test_14'})\nCREATE (:Test {value: 'test_15'})\nCREATE (:Test {value: 'test_16'})\nCREATE (:Test {value: 'test_17'})\nCREATE (:Test {value: 'test_18'})\nCREATE (:Test {value: 'test_19'})\nCREATE (:Test {value: 'test_20'})\nCR

....``` and it continues like that for thousands of more lines :)

[dominique-vassard]

I'm on it. Seems that I've been a little bit too optimistic regarding performance. I've raised the time limit. Now, all green!

But we should keep an eye oh this transaction test that randomly fails.

test/transaction_test.exs:72

     ** (MatchError) no match of right hand side value: {:ok, %Bolt.Sips.Response{bookmark: nil, fields: ["x"], notifications: [], plan: nil, profile: nil, records: [], results: [], stats: [], type: "r"}}

[florinpatrascu]

awesome, and yes, we'll monitor that test. Thank you!

[sascha-wolf]

I just wanna pop in and say that I also discovered this bottleneck last week and had plans to provide a fix. So thanks for doing this already.

I have one note though: the Elixir Library Guidelines list using exceptions for control flow as an anti-pattern. As such I would suggest to keep this in mind, or put it in a list somewhere to be eventually refactored.

[dominique-vassard]

Hi Sascha, THe problem we have here to follow this particular guidelines is that Kernel.apply either works or raises. The possible workaround is to check to avoid the raising error would be.... to check if module / function exists which is done by Code.ensure_loaded. Additionnally, narrowing the rescue to UndefinedFunctionError allows all other exception to be raised.

And to be honest, this fix was heavily inspired by an Ecto commi who fi the same issue: https://github.com/elixir-ecto/ecto/commit/46674c3d31f55480e8cedba1e1d2e651cf2ae941

A possible refactor that I have in mind in to use Code.ensure_loaded once at library setup and save the resut in some Agent, and use it when necessary. But I think it's a little bit complicated.

One guideline which also worth a look is the one about configuration: https://hexdocs.pm/elixir/library-guidelines.html#avoid-application-configuration Having bolt_sips configured on a per-app basis would be great.

[florinpatrascu]

Having bolt_sips configured on a per-app basis would be great.

Application configuration is optional, the only prerequisite being: to start the driver (under a supervision tree, ideally). And actually, a lot of refactoring went into paving the road to a better configuration support. If you look at its effects, for example: https://github.com/florinpatrascu/bolt_sips/blob/master/test/support/conn_routing_case.ex#L8-L20 Without this support, having the ability to connect to multiple dbs in the same time, from the same app, would be practically impossible :) Probably we should make this message more visible, in the docs?! (https://github.com/florinpatrascu/bolt_sips/blob/master/docs/features/configuration.md#multi-tenancy)

[florinpatrascu]

@sascha-wolf - and we'll never refuse any code contributions (PR) if you spot any weaknesses or opportunities to improve the driver ;)

[florinpatrascu]

ahh and if you feel that's something we want to act upon and it's contained in this thread, let's extract it in its own issue rather than having it buried in a close PR message, my 0.02CAD =)