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

Skip pretty-printing of unused accumulator field #3637

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Apr 22, 2022

Another low-hanging fruit. This field is actually never used anywhere in MongooseIM code so it could even be dropped altogether, but at the very least we can just skip the to_binary until it is used, if ever, and in the logger we now have selective pretty-printers, so we can drop it altogether.

@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

Merging #3637 (6189d6d) into master (f5b511c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3637      +/-   ##
==========================================
+ Coverage   80.99%   81.00%   +0.01%     
==========================================
  Files         427      427              
  Lines       31965    31965              
==========================================
+ Hits        25889    25894       +5     
+ Misses       6076     6071       -5     
Impacted Files Coverage Δ
src/mongoose_acc.erl 92.64% <100.00%> (ø)
src/elasticsearch/mongoose_elasticsearch.erl 76.92% <0.00%> (-7.70%) ⬇️
src/mam/mod_mam_elasticsearch_arch.erl 85.08% <0.00%> (-1.76%) ⬇️
src/rdbms/mongoose_rdbms.erl 62.54% <0.00%> (-1.10%) ⬇️
src/domain/mongoose_domain_loader.erl 89.28% <0.00%> (-0.90%) ⬇️
src/pubsub/mod_pubsub_db_mnesia.erl 92.43% <0.00%> (-0.43%) ⬇️
src/mod_muc_log.erl 63.21% <0.00%> (ø)
src/global_distrib/mod_global_distrib_receiver.erl 80.72% <0.00%> (+1.20%) ⬆️
src/inbox/mod_inbox_rdbms_async.erl 71.64% <0.00%> (+1.49%) ⬆️
...rc/smart_markers/mod_smart_markers_rdbms_async.erl 88.88% <0.00%> (+3.70%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5b511c...6189d6d. Read the comment docs.

@mongoose-im

This comment was marked as outdated.

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I think this is a complication. There shouldn't be a tuple with a function in a field called origin_stanza.

@NelsonVides
Copy link
Collaborator Author

Honestly, I think this is a complication. There shouldn't be a tuple with a function in a field called origin_stanza.

So should we remove this field instead? This origin_stanza is never used, and so that to_binary is a wasted computation. The logger is printing the element again anyway (

format_stanza_map(#{element := Elem, from_jid := From, to_jid := To}) ->
), because the element might have changed with all the appends like mam's id and so on.

@chrzaszcz
Copy link
Member

chrzaszcz commented Apr 26, 2022

So should we remove this field instead? This origin_stanza is never used, and so that to_binary is a wasted computation.

The whole point was to have the original stanza in the Acc for debugging - to be able to find out how it looked like before it was changed by the server. I think that to_binary might be used either because it is easier to read a binary stanza or because it can consume less memory than the whole nested Erlang structure of an xmlel.

What I don't understand is why the fun was added - for me it makes no sense. If putting just an element there is noticeably faster and the logs don't look worse (and there is no memory penalty), we could change it. I wouldn't remove the field, because it was added there for a reason... unless the original implementer agrees.

@NelsonVides NelsonVides changed the title Make to_binary lazy if it is actually never used Skip pretty-printing of unused accumulator field Apr 26, 2022
@mongoose-im

This comment was marked as outdated.

By now we have a pretty printer in the logger, and creating accumulators
is a hot operation, so we can skip this field when it is not needed.
@mongoose-im
Copy link
Collaborator

mongoose-im commented Apr 26, 2022

small_tests_23 / small_tests / 6189d6d
Reports root / small


small_tests_24 / small_tests / 6189d6d
Reports root / small


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 6189d6d
Reports root/ big
OK: 2860 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 6189d6d
Reports root/ big
OK: 2860 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / 6189d6d
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 6189d6d
Reports root/ big
OK: 2860 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / 6189d6d
Reports root/ big
OK: 2843 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / 6189d6d
Reports root/ big
OK: 1506 / Failed: 0 / User-skipped: 401 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / 6189d6d
Reports root/ big
OK: 1547 / Failed: 0 / User-skipped: 360 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / 6189d6d
Reports root/ big
OK: 3234 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 6189d6d
Reports root/ big
OK: 1854 / Failed: 0 / User-skipped: 368 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 6189d6d
Reports root/ big
OK: 3234 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / 6189d6d
Reports root/ big
OK: 3229 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / 6189d6d
Reports root/ big
OK: 3234 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 6189d6d
Reports root/ big
OK: 1697 / Failed: 0 / User-skipped: 367 / Auto-skipped: 0

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👍

@@ -346,21 +346,18 @@ large_event_dont_crash_formatter(_Config) ->
%%

example_acc(Body) ->
Elem = {xmlel, <<"message">>,
[{<<"type">>, <<"chat">>}, {<<"id">>, <<"1111">>}],
[{xmlel, <<"body">>,[], [{xmlcdata, Body}]}]},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nit-picky, I can merge as it is.

Suggested change
[{xmlel, <<"body">>,[], [{xmlcdata, Body}]}]},
[{xmlel, <<"body">>, [], [{xmlcdata, Body}]}]},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm I think we can spare an extra CI run here, there were no spaces before, I tried adding them and missed one... 😅

@chrzaszcz chrzaszcz merged commit c53ce7a into master Apr 26, 2022
@chrzaszcz chrzaszcz deleted the perf/origin_stanza_acc branch April 26, 2022 12:17
@Premwoik Premwoik added this to the 5.1.0 milestone May 25, 2022
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.

4 participants