basho / erlang_js

A linked-in driver for Erlang to Mozilla's Spidermonkey Javascript runtime.
Apache License 2.0
238 stars 88 forks source link

Erlang 18.0 now() fix. #50

Closed billstclair closed 8 years ago

billstclair commented 8 years ago

Make erlang_js work in OTP 18.

This fixes the OTP 18 deprecation warnings for erlang:now(), which are fatal due to warnings_as_errors in the erl_opts setting in rebar.config.

The erlang_js_timestamp.erl code is from http://www.erlang.org/doc/apps/erts/time_compat.erl

The modified erlang_js application passes "make test" in basho/otp branches basho-otp16, basho-otp-17, and basho-otp-18. "All 55 tests passed."

billstclair commented 8 years ago

I notice that mkurkov fixed this in #49 by changing erlang:now() to os:timestamp(). That has slightly different semantics. erlang:now() is guaranteed to always return a unique value. os:timestamp() returns the wall clock time. I doubt that matters here, since it's just timing a benchmark, but the Erlang folks recommend replacing erlang:now() with erlang:timestamp() in 18.0 and leaving it as is before that, which is what my version does.

I do like the simplicity of mkurkov's fix, though.

And my testing shows that os:timestamp() DOES return a unique microsecond value, though it is not so documented.

Fred Hebert's take: apparently erlang:now() has to grab a lock so it can increment a shared value. os:timestamp() just returns the time.

tburghart commented 8 years ago

The correct way to time execution of an operation is with the timer:tc functions, whose implementations differ to use appropriate timekeeping primitives across OTP releases.

The approach in this PR should not be used.

hmmr commented 8 years ago

Agree with @tburghart, specifically in that here the code is about timing a function call, and not the uniqueness of the value returned. by now() or os:timestamp(). There's a competing PR, containing the right thing: https://github.com/basho/erlang_js/pull/55. Informative explanations by @billstclair, though.

lemenkov commented 8 years ago

Fixed in #55 and merged. So I believe someone should close this PR.

hmmr commented 8 years ago

Closing, then. Thank you for your contributions guys.