Optimize tlsv1.3 #9305
Optimize tlsv1.3 #9305
Conversation
And remove the installation of src header files previously used, I do this to avoid installing src headers in tests which can mess up things during development and local testing. Also remove some logging.
Some minor optimizations
CT Test Results 1 files 11 suites 4m 7s ⏱️ Results for commit 20612c9. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
Readuce performance overhead
Avoid several external calls for every data package. Remove debug code for application data, i.e. fast path.
dgud
force-pushed
the
dgud/ssl/optimize-tlsv1.3/OTP-19430
branch
from
77457a5 to
20612c9
Compare
| @@ -47,7 +47,7 @@ setup(Name) -> | |||
| PeerOptions = | |||
| #{name => NameStr, | |||
| host => Host}, | |||
| ?CT_LOG("PeerOptions: ~p~n", [PeerOptions]), | |||
| %% io:format("PeerOptions: ~p~n", [PeerOptions]), | |||
There was a problem hiding this comment.
Why are we doing io:format here in this commented code?
I think we should be consistent through the file. Question is if we should keep it at all? Although it bothers me less in test code, I think in the normal case we do not want commented out code.
There was a problem hiding this comment.
I also dislike commented out code.
However, I like when a module has a DEBUG flag or macro that could be enabled if needed.
Maybe it would be better and easy to do than leaving commented out lines here and there?
Maybe CT_LOG should have a variant working conditionally? or maybe have ?DBG wrapping macro
?DBG(?CT_LOG()) so argument macro is only included when debug is true?
There was a problem hiding this comment.
This is just a mistake and should not be included, when running measurements 1000 times you don't want to bother
with starting common tests and look at webpages.
| @@ -363,6 +363,8 @@ wait_eoed(Type, Msg, State) -> | |||
| term(), #state{}) -> | |||
| gen_statem:state_function_result(). | |||
| %%-------------------------------------------------------------------- | |||
| connection(info, Msg, State) -> | |||
| tls_gen_connection:handle_info(Msg, connection, State); | |||
There was a problem hiding this comment.
Same question as for client.
There was a problem hiding this comment.
To avoid those 2 function calls?
There was a problem hiding this comment.
But maybe it should be gen_info that is called, lets continue this when we see how #9273 turns out. To ensure that error handling is done correctly.
| @@ -568,7 +568,6 @@ validate_tls_record_length(Versions, {_,Size0,_} = Q0, MaxFragLen, | |||
| %% Complete record | |||
| {Fragment, Q} = binary_from_front(Length, Q0), | |||
| Record = #ssl_tls{type = Type, version = Version, fragment = Fragment}, | |||
| ssl_logger:debug(get(log_level), inbound, 'record', Record), | |||
There was a problem hiding this comment.
We might want to do this during handshaking, or we will loose OpenSSL debug logging compatibility?!
There was a problem hiding this comment.
Keep this but Comment out the code and comment why it is outcommented.
| @@ -47,7 +47,7 @@ setup(Name) -> | |||
| PeerOptions = | |||
| #{name => NameStr, | |||
| host => Host}, | |||
| ?CT_LOG("PeerOptions: ~p~n", [PeerOptions]), | |||
| %% io:format("PeerOptions: ~p~n", [PeerOptions]), | |||
There was a problem hiding this comment.
I also dislike commented out code.
However, I like when a module has a DEBUG flag or macro that could be enabled if needed.
Maybe it would be better and easy to do than leaving commented out lines here and there?
Maybe CT_LOG should have a variant working conditionally? or maybe have ?DBG wrapping macro
?DBG(?CT_LOG()) so argument macro is only included when debug is true?
| ConnState = ConnState0#{security_parameters => SecParams, | ||
| cipher_state => CipherState, | ||
| sequence_number => 0}, | ||
| ConnStateOld = maps:remove(aead_handle, ConnStateOld0), |
There was a problem hiding this comment.
why using Old suffix when you have indexed variables used already?
| {Content, CipherTag} = | ||
| ssl_cipher:aead_encrypt(BulkCipherAlgo, Key, Nonce, Fragment, AAD, TagLen), | ||
| <<Content/binary, CipherTag/binary>>. | ||
| %% crypto:exor(<<0:(bit_size(IV)-64),?UINT64(Seq)>>, IV). |
There was a problem hiding this comment.
why crypto call is commented above and replaced with hard code below?
are such low level operations wanted in ssl code? looks like crypto code?
if needed here, maybe explain reason in code comment?
There was a problem hiding this comment.
Why not do it in erlang?
It is faster (if jit is running).
There was a problem hiding this comment.
I think we should prefer doing things in erlang. This is no cryptographic algorithm. Actually Dan touched it last.
dc9ec91 (Péter Dimitrov 2019-01-08 13:36:53 +0100 294) %% per-record nonce.
dc9ec91 (Péter Dimitrov 2019-01-08 13:36:53 +0100 295) nonce(Seq, IV) ->
293db3c (Dan Gudmundsson 2024-01-17 09:22:03 +0100 296) crypto:exor(<<0:(bit_size(IV)-64),?UINT64(Seq)>>,
Some optimizations for tls.