Optimise just_tls with hibernate_after #4447
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4447 +/- ##
==========================================
- Coverage 85.35% 85.34% -0.02%
==========================================
Files 550 550
Lines 33883 33885 +2
==========================================
- Hits 28921 28919 -2
- Misses 4962 4966 +4 ☔ View full report in Codecov by Sentry. |
NelsonVides
force-pushed
the
just_ssl_optimisations
branch
2 times, most recently
from
b3be0bb to
724d44b
Compare
NelsonVides
force-pushed
the
just_ssl_optimisations
branch
from
724d44b to
b7339b3
Compare
Note that this can reduce memory consumption by a factor of 4x. Also clean initial calculations of the SSL options, to generate less intermediate garbage when creating the proplist.
NelsonVides
force-pushed
the
just_ssl_optimisations
branch
from
b7339b3 to
dee13d6
Compare
NelsonVides
force-pushed
the
just_ssl_optimisations
branch
from
dee13d6 to
80e5e94
Compare
|
elasticsearch_and_cassandra_27 / elasticsearch_and_cassandra_mnesia / 80e5e94 small_tests_26 / small_tests / 80e5e94 small_tests_27 / small_tests / 80e5e94 small_tests_27_arm64 / small_tests / 80e5e94 ldap_mnesia_26 / ldap_mnesia / 80e5e94 dynamic_domains_pgsql_mnesia_26 / pgsql_mnesia / 80e5e94 dynamic_domains_mysql_redis_27 / mysql_redis / 80e5e94 dynamic_domains_pgsql_mnesia_27 / pgsql_mnesia / 80e5e94 ldap_mnesia_27 / ldap_mnesia / 80e5e94 internal_mnesia_27 / internal_mnesia / 80e5e94 mysql_redis_27 / mysql_redis / 80e5e94 dynamic_domains_mssql_mnesia_27 / odbc_mssql_mnesia / 80e5e94 last_SUITE:valid_queries:last_online_user{error,
{test_case_failed,
{has_stanzas_but_shouldnt,
{client,<<"[email protected]/res1">>,
escalus_tcp,<0.125004.0>,
[{event_manager,<0.125003.0>},
{server,<<"domain.example.com">>},
{username,<<"alicE_unnamed_3530">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.125003.0>},
{server,<<"domain.example.com">>},
{username,<<"alicE_unnamed_3530">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"alice_unnamed_3530">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"alicE_unnamed_3530">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"ff6f491da9f47336">>}]},
[{xmlel,<<"presence">>,
[{<<"from">>,<<"[email protected]/res1">>},
{<<"to">>,<<"[email protected]/res1">>}],
[]}]}}}pgsql_mnesia_26 / pgsql_mnesia / 80e5e94 pgsql_cets_27 / pgsql_cets / 80e5e94 cockroachdb_cets_27 / cockroachdb_cets / 80e5e94 pubsub_SUITE:dag+manage_subscriptions:retrieve_node_subscriptions_test{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,
<<"alice_retrieve_node_subscriptions_test_2934@localhost/res1">>,
escalus_tcp,<0.104154.0>,
[{event_manager,<0.104123.0>},
{server,<<"localhost">>},
{username,
<<"alicE_retrieve_node_subscriptions_test_2934">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.104123.0>},
{server,<<"localhost">>},
{username,
<<"alicE_retrieve_node_subscriptions_test_2934">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,
<<"alice_retrieve_node_subscriptions_test_2934">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,
<<"alicE_retrieve_node_subscriptions_test_2934">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"a0375d1f445842e0">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{pubsub_tools,receive_response,3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,433}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"/h...mssql_mnesia_27 / odbc_mssql_mnesia / 80e5e94 pgsql_mnesia_27 / pgsql_mnesia / 80e5e94 dynamic_domains_mssql_mnesia_27 / odbc_mssql_mnesia / 80e5e94 |
| {dummy_ref, SSLOpts} = format_opts_with_ref(Opts, FailIfNoPeerCert), | ||
| SSLOpts. | ||
| make_cowboy_ssl_opts(#{verify_mode := Mode} = Opts) -> | ||
| SslOpts = format_opts(Opts, fail_if_no_peer_cert), |
There was a problem hiding this comment.
I think it's confusing, because the type of the second argument looks like false | fail_if_no_peer_cert, which is unexpected. I think this should be either a boolean (i.e. true | false) or two descriptive atoms. Alternatively, make_ssl_opts could pass Opts#{disconnect_on_failure => false}, avoiding this extra argument altogether.
| fail_if_no_peer_cert_opt(#{verify_mode := peer}) -> true; | ||
| fail_if_no_peer_cert_opt(#{verify_mode := selfsigned_peer}) -> true; | ||
| fail_if_no_peer_cert_opt(#{}) -> false. | ||
| fail_if_no_peer_cert_opts(Opts, #{}, false) -> |
There was a problem hiding this comment.
For me, this function got too complex. Please see the comment above regarding the possible simplification.
| SslOpts0 = maps:to_list(maps:with(ssl_option_keys(), Opts)), | ||
| SslOpts1 = sni_opts(SslOpts0, Opts), | ||
| SslOpts2 = verify_opts(SslOpts1, Opts), | ||
| SslOpts3 = hibernate_opts(SslOpts2, Opts), | ||
| fail_if_no_peer_cert_opts(SslOpts3, Opts, FailIfNoPeerCert). |
There was a problem hiding this comment.
Why not make the functions just return their options in lists, and do a lists:flatmap over the list of functions?
There was a problem hiding this comment.
This one is actually for performance, this is code that will be called on every single new connection, so I wanted to avoid making too many temporary lists, doing a flatmap would copy all left-sides of the ++ operations. It might be such a tiny thing, lists here have just one or two elements, but still, the code looks very readable to me anyway.
There was a problem hiding this comment.
For me, as long as there is no sign of it being inefficient, I would favor simplicity 🤷♂️
There was a problem hiding this comment.
Yeah I know but this one looks like the simplest and most idiomatic to me xD It's what a pipe operator (like in Elixir) would have done so elegantly, in Erlang we only needed to get more verbose 🥲 Anyway, just a matter of styling 🤷🏽♂️
There was a problem hiding this comment.
Your version is just more error-prone, because you can e.g. mess up the 1, 2, 3 suffixes or forget | Opts in one of the functions. I tend to use map when always one element is returned, flatmap when lists need to be concatenated, and fold when the list can be manipulated. By using the least-permissive option, I limit bugs (including future ones). Moreover, such code looks more like functional programming, and less like an imperative code. Anyway, I will not change this, as you seem to have a strong opinion here.
There was a problem hiding this comment.
No no, opinion is not strong, I'm just saying this is what I find most idiomatic, and perfectly functional: piping things. The number suffixes are an unfortunate flaw of not having a proper pipe operator in Erlang and that indeed makes it potentially more error prone, unfortunately, and for that reason I'm not strong on this one. If we had a proper pipe operator like Elixir has then I'd reeeally insist, otherwise this is just my habit 😄
So yeah, please throw into the PR a proposal with flatmap, happy to have a look! 👌🏽
The SSL processes were not garbage-collected sufficiently often, while the c2s is. With this change, we just pass the
hibernate_aftervalue of the c2s process to the SSL ones. Local load-tests with amoc show a decrease of 4x, from ~80KB per connection to ~20KB (in comparison,fast_tlsshows ~6KB per connection 🥲).Also clean initial calculations of the SSL options, to generate less intermediate garbage when creating the proplist and improve a bit the readability of the helpers around
verify_fun.