Replace p1_time_compat with direct OTP functions #2498

NelsonVides · 2019-10-17T12:49:33Z

Pretty much a mechanical change. Need to see results from CI.

codecov · 2019-10-17T13:29:49Z

Codecov Report

Merging #2498 into master will decrease coverage by 0.05%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
- Coverage   78.93%   78.88%   -0.06%     
==========================================
  Files         340      340              
  Lines       29329    29324       -5     
==========================================
- Hits        23152    23131      -21     
- Misses       6177     6193      +16

Impacted Files	Coverage Δ
src/mod_commands.erl	`96.07% <ø> (ø)`	⬆️
src/jingle_sip/mod_jingle_sip_backend.erl	`80% <ø> (ø)`	⬆️
src/sasl/cyrsasl_anonymous.erl	`75% <ø> (ø)`	⬆️
src/mod_offline_mnesia.erl	`45.31% <0%> (ø)`	⬆️
src/gen_iq_handler.erl	`59.09% <0%> (ø)`	⬆️
src/mod_muc.erl	`75.13% <0%> (ø)`	⬆️
src/auth/ejabberd_auth_external.erl	`32.38% <0%> (+0.3%)`	⬆️
src/mod_time.erl	`91.3% <100%> (ø)`	⬆️
...rc/stream_management/stream_management_stale_h.erl	`71.05% <100%> (ø)`	⬆️
src/global_distrib/mod_global_distrib_worker.erl	`80.95% <100%> (ø)`	⬆️
... and 35 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 db22255...318cf32. Read the comment docs.

mongoose-im · 2019-10-17T14:20:09Z

7343.1 / Erlang 22.0 / small_tests / 05f0f10
Reports root / small

mongoose-im · 2019-10-17T14:34:38Z

7344.1 / Erlang 22.0 / small_tests / 1347427
Reports root / small

7344.2 / Erlang 22.0 / internal_mnesia / 1347427
Reports root/ big
OK: 1267 / Failed: 0 / User-skipped: 136 / Auto-skipped: 0

7344.3 / Erlang 22.0 / odbc_mssql_mnesia / 1347427
Reports root/ big
OK: 2484 / Failed: 0 / User-skipped: 199 / Auto-skipped: 0

7344.4 / Erlang 22.0 / mysql_redis / 1347427
Reports root/ big
OK: 2479 / Failed: 0 / User-skipped: 204 / Auto-skipped: 0

7344.7 / Erlang 22.0 / elasticsearch_and_cassandra_mnesia / 1347427
Reports root/ big
OK: 328 / Failed: 0 / User-skipped: 28 / Auto-skipped: 0

7344.6 / Erlang 22.0 / ldap_mnesia / 1347427
Reports root/ big
OK: 1236 / Failed: 0 / User-skipped: 167 / Auto-skipped: 0

7344.5 / Erlang 22.0 / riak_mnesia / 1347427
Reports root/ big
OK: 1398 / Failed: 0 / User-skipped: 145 / Auto-skipped: 0

7344.9 / Erlang 21.3 / pgsql_mnesia / 1347427
Reports root/ big / small
OK: 2497 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

fenek

There are some p1_time_compat leftovers in small tests. :)

michalwski

Thanks for the changes! Few things:

I can see there is many deprecated time units used: http://erlang.org/doc/man/erlang.html#type-deprecated_time_unit Since the oldest supported Erlang/OTP is 21.2 now, we can use only the correct time units like described here: http://erlang.org/doc/man/erlang.html#type_time_unit I commented many lines regarding this, but not all of them
In our rebar.config we still have this lib usec lib (https://github.com/esl/usec) which was created to deal with conversions from the timestamp format to micro seconds. Do we still use this lib? Can it be replaced with proper use of erlang:system_time?

michalwski · 2019-10-17T18:16:13Z

src/ejabberd_s2s_out.erl

@@ -1151,7 +1151,7 @@ wait_before_reconnect(StateData) ->
                undefined_delay ->
                    %% The initial delay is random between 1 and 15 seconds
                    %% Return a random integer between 1000 and 15000
-                    {_, _, MicroSecs} = p1_time_compat:timestamp(),
+                    MicroSecs = erlang:system_time(microsecond),


Taking into account the comment above, current implementation is less random as far as I can reason about it. Also since we want a random number, maybe there is a better way than calculate it from a timestamp?

I'm not sure this makes much of a difference, it's not a place were cryptographically safe randomization makes any difference. We just want to wait some random amount of time between 1 an 15 seconds. The fact that this function might run at unpredictable times makes it sufficiently random for our purposes I think 🙁
An option would be to do X = rand:uniform(14000), X+1000, but the performance is pretty much the same (I've just measure it on my laptop with timer:tc over a 10M repeats), and rand is saving seeds into the process dictionary. If you prefer that I can quickly change it, but I don't think it matters much 🤷‍♂️

michalwski · 2019-10-17T18:22:12Z

src/global_distrib/mod_global_distrib_bounce.erl

@@ -39,7 +39,7 @@
 -spec start(Host :: jid:lserver(), Opts :: proplists:proplist()) -> any().
 start(Host, Opts0) ->
    ResendAfterMs = proplists:get_value(resend_after_ms, Opts0, 200),
-    ResendAfter = p1_time_compat:convert_time_unit(ResendAfterMs, milli_seconds, native),
+    ResendAfter = erlang:convert_time_unit(ResendAfterMs, milli_seconds, native),


The milli_seconds is a deprecated time unit. See: http://erlang.org/doc/man/erlang.html#type-deprecated_time_unit it was used in Erlang/OTP 18 as far as I remember and changed to millisecond in later. See: http://erlang.org/doc/man/erlang.html#type_time_unit

michalwski · 2019-10-17T18:22:33Z

src/global_distrib/mod_global_distrib_bounce.erl

@@ -103,10 +103,10 @@ maybe_store_message({From, To, Acc0, Packet} = FPacket) ->
            ?DEBUG("Storing global message id=~s from=~s to=~s to "
                   "resend after ~B ms (bounce_ttl=~B)",
                   [ID, jid:to_binary(From), jid:to_binary(To),
-                    p1_time_compat:convert_time_unit(opt(resend_after), native, milli_seconds),
+                    erlang:convert_time_unit(opt(resend_after), native, milli_seconds),


Deprecated milli_seconds

michalwski · 2019-10-17T18:23:06Z

src/global_distrib/mod_global_distrib_connection.erl

-    QueueTimeNative = p1_time_compat:monotonic_time() - Stamp,
-    QueueTimeUS = p1_time_compat:convert_time_unit(QueueTimeNative, native, micro_seconds),
+    QueueTimeNative = erlang:monotonic_time() - Stamp,
+    QueueTimeUS = erlang:convert_time_unit(QueueTimeNative, native, micro_seconds),


Deprecated micro_seconds.

michalwski · 2019-10-17T18:23:16Z

src/global_distrib/mod_global_distrib_connection.erl

    mongoose_metrics:update(global, ?GLOBAL_DISTRIB_SEND_QUEUE_TIME(ToHost), QueueTimeUS),
-    ClockTime = p1_time_compat:system_time(micro_seconds),
+    ClockTime = erlang:system_time(micro_seconds),


Deprecated micro_seconds.

michalwski · 2019-10-17T18:25:09Z

src/mam/mod_mam_utils.erl

@@ -194,7 +194,7 @@ microseconds_to_datetime(MicroSeconds) when is_integer(MicroSeconds) ->
 -spec generate_message_id() -> integer().
 generate_message_id() ->
    {ok, NodeId} = ejabberd_node_id:node_id(),
-    CandidateStamp = p1_time_compat:os_system_time(micro_seconds),
+    CandidateStamp = erlang:system_time(micro_seconds),


Deprecated micro_seconds.

michalwski · 2019-10-17T18:25:33Z

src/mod_bosh_socket.erl

@@ -544,7 +544,7 @@ schedule_report(Ack, #state{sent = Sent} = S) ->
    ReportRid = Ack + 1,
    try
        {ReportRid, TimeSent, _} = lists:keyfind(ReportRid, 1, Sent),
-        ElapsedTimeMillis = p1_time_compat:monotonic_time(milli_seconds) - TimeSent,
+        ElapsedTimeMillis = erlang:monotonic_time(milli_seconds) - TimeSent,


Deprecated milli_seconds.

michalwski · 2019-10-17T18:25:45Z

src/mod_bosh_socket.erl

@@ -670,7 +670,7 @@ send_to_handler({_, Pid}, #xmlel{name = <<"body">>} = Wrapped, State) ->
    send_wrapped_to_handler(Pid, Wrapped, State);
 send_to_handler({Rid, Pid}, Data, State) ->
    {Wrapped, NS} = bosh_wrap(Data, Rid, State),
-    NS2 = cache_response({Rid, p1_time_compat:monotonic_time(milli_seconds), Wrapped}, NS),
+    NS2 = cache_response({Rid, erlang:monotonic_time(milli_seconds), Wrapped}, NS),


Deprecated milli_seconds.

michalwski · 2019-10-17T18:26:03Z

src/mod_commands.erl

@@ -379,7 +379,7 @@ lookup_recent_messages(ArcJID, WithJID, Before, Limit) ->
               rsm => #rsm_in{direction = before, id = undefined}, % last msgs
               start_ts => undefined,
               end_ts => Before * 1000000,
-               now => p1_time_compat:os_system_time(micro_seconds),
+               now => os:system_time(micro_seconds),


Deprecated micro_seconds.

michalwski · 2019-10-17T18:28:23Z

src/mod_last.erl

@@ -131,7 +131,7 @@ process_local_iq(_From, _To, Acc,
 get_node_uptime() ->
    case ejabberd_config:get_local_option(node_start) of
        {_, _, _} = StartNow ->
-            now_to_seconds(p1_time_compat:timestamp()) - now_to_seconds(StartNow);
+            erlang:system_time(seconds) - now_to_seconds(StartNow);


I realized just now, that seconds is also deprecated time unit :) the correct one is just second without the s. I'm not going back to all the previous places where seconds unit were used, I sure you'll find them easily. Also I stop pointing other deprecated time units unless I spot one I didn't comment yet.

NelsonVides · 2019-10-17T20:42:13Z

@fenek small tests done, sorry I forgot that 😅
@michalwski about user, I was having a look at that, and I think it gets quite complicated, I think out of the scope of p1_time_compat. It's used in tons of places and the values it returns are passed along a ton of others, not a mechanical change perhaps 😞
Also, the damn deprecated time_units, that surprised me xD

mongoose-im · 2019-10-17T20:53:13Z

7348.1 / Erlang 22.0 / small_tests / 98d94a6
Reports root / small

7348.2 / Erlang 22.0 / internal_mnesia / 98d94a6
Reports root/ big
OK: 1269 / Failed: 0 / User-skipped: 136 / Auto-skipped: 0

7348.3 / Erlang 22.0 / odbc_mssql_mnesia / 98d94a6
Reports root/ big
OK: 2486 / Failed: 0 / User-skipped: 199 / Auto-skipped: 0

7348.4 / Erlang 22.0 / mysql_redis / 98d94a6
Reports root/ big
OK: 2481 / Failed: 0 / User-skipped: 204 / Auto-skipped: 0

7348.7 / Erlang 22.0 / elasticsearch_and_cassandra_mnesia / 98d94a6
Reports root/ big
OK: 328 / Failed: 0 / User-skipped: 28 / Auto-skipped: 0

7348.6 / Erlang 22.0 / ldap_mnesia / 98d94a6
Reports root/ big
OK: 1238 / Failed: 0 / User-skipped: 167 / Auto-skipped: 0

7348.5 / Erlang 22.0 / riak_mnesia / 98d94a6
Reports root/ big
OK: 1400 / Failed: 0 / User-skipped: 145 / Auto-skipped: 0

7348.9 / Erlang 21.3 / pgsql_mnesia / 98d94a6
Reports root/ big / small
OK: 2499 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

mongoose-im · 2019-10-18T06:59:09Z

7349.1 / Erlang 22.0 / small_tests / f407a76
Reports root / small

7349.2 / Erlang 22.0 / internal_mnesia / f407a76
Reports root/ big
OK: 1269 / Failed: 0 / User-skipped: 136 / Auto-skipped: 0

7349.3 / Erlang 22.0 / odbc_mssql_mnesia / f407a76
Reports root/ big
OK: 2486 / Failed: 0 / User-skipped: 199 / Auto-skipped: 0

7349.4 / Erlang 22.0 / mysql_redis / f407a76
Reports root/ big
OK: 2481 / Failed: 0 / User-skipped: 204 / Auto-skipped: 0

7349.7 / Erlang 22.0 / elasticsearch_and_cassandra_mnesia / f407a76
Reports root/ big
OK: 328 / Failed: 0 / User-skipped: 28 / Auto-skipped: 0

7349.6 / Erlang 22.0 / ldap_mnesia / f407a76
Reports root/ big
OK: 1238 / Failed: 0 / User-skipped: 167 / Auto-skipped: 0

7349.5 / Erlang 22.0 / riak_mnesia / f407a76
Reports root/ big
OK: 1400 / Failed: 0 / User-skipped: 145 / Auto-skipped: 0

7349.9 / Erlang 21.3 / pgsql_mnesia / f407a76
Reports root/ big / small
OK: 2499 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

michalwski

Looks good to be merged now.

I think we can live with usec for now. Maybe sometime in the future we'll remove it as well.

Regarding the randomization, it can be like it is now. It just looks a bit awkward to see random number is calculated from time but it does the job. Most probably back then it was far more efficient then using proper random generator.

NelsonVides added WIP 🚧 waiting-for-ci labels Oct 17, 2019

NelsonVides force-pushed the out_p1_time_compat branch from 09ba93d to cd5702e Compare October 17, 2019 13:03

aleklisi approved these changes Oct 17, 2019

View reviewed changes

NelsonVides force-pushed the out_p1_time_compat branch from cd5702e to 0e8e670 Compare October 17, 2019 13:29

This comment has been minimized.

Sign in to view

NelsonVides added 2 commits October 17, 2019 16:20

Replace p1_time_compat with direct OTP functions

f134932

Fix monotonic_time by system_time

480bf8f

NelsonVides force-pushed the out_p1_time_compat branch from 666f303 to 480bf8f Compare October 17, 2019 14:21

NelsonVides added waiting-for-review and removed WIP 🚧 waiting-for-ci labels Oct 17, 2019

fenek requested changes Oct 17, 2019

View reviewed changes

michalwski suggested changes Oct 17, 2019

View reviewed changes

Remove p1_time_compat from small_tests

a189571

NelsonVides added 2 commits October 18, 2019 08:46

updating deprecated_time_unit()

0230250

Get rid of now_to_Second old trick

318cf32

NelsonVides force-pushed the out_p1_time_compat branch from 8c845aa to 318cf32 Compare October 18, 2019 06:46

NelsonVides requested review from fenek and michalwski October 18, 2019 06:47

michalwski approved these changes Oct 18, 2019

View reviewed changes

michalwski merged commit 2a3b68c into master Oct 18, 2019

michalwski deleted the out_p1_time_compat branch October 18, 2019 08:20

michalwski added this to the 3.6.0 milestone Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace p1_time_compat with direct OTP functions #2498

Replace p1_time_compat with direct OTP functions #2498

NelsonVides commented Oct 17, 2019

codecov bot commented Oct 17, 2019 •

edited

Loading

This comment has been minimized.

mongoose-im commented Oct 17, 2019

mongoose-im commented Oct 17, 2019 •

edited

Loading

fenek left a comment

michalwski left a comment

michalwski Oct 17, 2019

NelsonVides Oct 18, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

michalwski Oct 17, 2019

NelsonVides commented Oct 17, 2019

mongoose-im commented Oct 17, 2019 •

edited

Loading

mongoose-im commented Oct 18, 2019 •

edited

Loading

michalwski left a comment

Replace p1_time_compat with direct OTP functions #2498

Replace p1_time_compat with direct OTP functions #2498

Conversation

NelsonVides commented Oct 17, 2019

codecov bot commented Oct 17, 2019 • edited Loading

Codecov Report

This comment has been minimized.

mongoose-im commented Oct 17, 2019

mongoose-im commented Oct 17, 2019 • edited Loading

fenek left a comment

Choose a reason for hiding this comment

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelsonVides commented Oct 17, 2019

mongoose-im commented Oct 17, 2019 • edited Loading

mongoose-im commented Oct 18, 2019 • edited Loading

michalwski left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2019 •

edited

Loading

mongoose-im commented Oct 17, 2019 •

edited

Loading

mongoose-im commented Oct 17, 2019 •

edited

Loading

mongoose-im commented Oct 18, 2019 •

edited

Loading