-
Notifications
You must be signed in to change notification settings - Fork 428
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
[markers] removals #3544
[markers] removals #3544
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
15314e9
to
617ed5a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
617ed5a
to
3f4066a
Compare
Codecov Report
@@ Coverage Diff @@
## master #3544 +/- ##
==========================================
+ Coverage 79.23% 79.26% +0.02%
==========================================
Files 421 421
Lines 32283 32307 +24
==========================================
+ Hits 25581 25607 +26
+ Misses 6702 6700 -2
Continue to review full report at Codecov.
|
This comment was marked as outdated.
This comment was marked as outdated.
3f4066a
to
7fbfedb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7fbfedb
to
8182a6c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
8182a6c
to
817f7cf
Compare
small_tests_24 / small_tests / 817f7cf small_tests_23 / small_tests / 817f7cf dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 817f7cf dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 817f7cf dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 817f7cf dynamic_domains_mysql_redis_24 / mysql_redis / 817f7cf ldap_mnesia_24 / ldap_mnesia / 817f7cf ldap_mnesia_23 / ldap_mnesia / 817f7cf internal_mnesia_24 / internal_mnesia / 817f7cf mysql_redis_24 / mysql_redis / 817f7cf pgsql_mnesia_23 / pgsql_mnesia / 817f7cf elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 817f7cf mssql_mnesia_24 / odbc_mssql_mnesia / 817f7cf pgsql_mnesia_24 / pgsql_mnesia / 817f7cf riak_mnesia_24 / riak_mnesia / 817f7cf |
Actually for this branch, I'm not sure if this goes to the feature branch or it it can go to master, it's fairly unrelated to any new behaviour, just adding all the removals that should have been there to begin with 🤔 |
thread => get_thread(Packet), | ||
timestamp => TS, | ||
type => undefined, | ||
id => undefined}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these set here actually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance: https://www.erlang.org/doc/efficiency_guide/maps.html
It's faster to create a map with all the keys it'll need and then modify its values, than to add keys to a map. Small maps are implemented as tuples, and adding keys to the map means having to copy the entire tuple with the keys again 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I thought it was about performance 😄
It's a funny case, if the compiler was really clever, it could see what will happen with this map and expand it already...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTP guys are very conservative about the compiler being too clever because it dirties tracing. They're only recently being more aggressive about optimising the jit compiler, more than the traditional interpreter.
Nevertheless I don't know what the compiler would do here, would be interesting to dump the assembly and see, but allow me to do that some other day xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sure, just a thought. And I agree, that in some special cases, crazy debugging, it could be a nasty surprise to see some unexpected fields being added by who knows whom. xD
small_tests_24 / small_tests / f83207a small_tests_23 / small_tests / f83207a dynamic_domains_mysql_redis_24 / mysql_redis / f83207a dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / f83207a dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / f83207a dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / f83207a vcard_SUITE:ro_full:retrieve_own_card{error,{test_case_failed,"Expected <<\"alice\">> got undefined\n"}} vcard_SUITE:ro_full:update_other_card{error,{test_case_failed,"Expected <<\"alice\">> got undefined\n"}} vcard_SUITE:ro_full:retrieve_others_card{error,{test_case_failed,"Expected <<\"bob\">> got undefined\n"}} ldap_mnesia_24 / ldap_mnesia / f83207a internal_mnesia_24 / internal_mnesia / f83207a pgsql_mnesia_23 / pgsql_mnesia / f83207a elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / f83207a mysql_redis_24 / mysql_redis / f83207a mssql_mnesia_24 / odbc_mssql_mnesia / f83207a pgsql_mnesia_24 / pgsql_mnesia / f83207a riak_mnesia_24 / riak_mnesia / f83207a |
f83207a
to
66595bd
Compare
small_tests_24 / small_tests / 66595bd small_tests_23 / small_tests / 66595bd dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 66595bd dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 66595bd dynamic_domains_mysql_redis_24 / mysql_redis / 66595bd ldap_mnesia_23 / ldap_mnesia / 66595bd dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 66595bd internal_mnesia_24 / internal_mnesia / 66595bd ldap_mnesia_24 / ldap_mnesia / 66595bd elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 66595bd pgsql_mnesia_23 / pgsql_mnesia / 66595bd pgsql_mnesia_24 / pgsql_mnesia / 66595bd mysql_redis_24 / mysql_redis / 66595bd mssql_mnesia_24 / odbc_mssql_mnesia / 66595bd riak_mnesia_24 / riak_mnesia / 66595bd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
This PR adds all the automatic removals, like the very important remove_domain (which requires changing the DB model), and also remove_user, forget_room, and removing a user from a room.