Closed borkdude closed 1 year ago
Hi @borkdude Michiel, thanks so much for the clear report and solution! Apologies for the trouble.
Could you please confirm that [com.taoensso/timbre "6.0.2-SNAPSHOT"]
now works correctly on Graal?
Will deploy 6.0.2 final as soon as I get your confirmation.
Relatedly, it'd be great to get a Graal compile included in Timbre's tests (and in my other projects) but I've never used Graal myself. Would you maybe be able to point me in the direction of a good example of how to do this?
Cheers!
Hi, I raised the question around this in the Clojurians slack after running into this issue, and 6.0.2-SNAPSHOT has fixed it for me. Thanks for the (extremely) quick turnaround on this!
@dekelpilli Thanks a lot for the confirmation Dekel! Apologies for the trouble.
[com.taoensso/timbre "6.0.2"]
is now up on Clojars 👍
Will keep this issue open until we have some kind of Graal unit test included (or we rule out that possibility).
Fix looks good to me. Why was the random number needed in the log macro in the first place?
@borkdude Good question.
Basically we want a unique id for each logging call site (/ individual logging form). These ids can be useful for rate-limiting, etc.
But the arguments for a logging form aren't necessarily unique, so to uniquely identify each form we can use something like a counter or random number/uuid/gensym/etc.
Hope that makes sense?
Yes. So a global counter would have been sufficient I guess. (rand)
or (.nextDouble ...)
can give you twice the same number (theoretically), a strictly increasing counter doesn't, although there the issue might be that eventually you will have to move to BigInteger: (inc' Long/MAX_VALUE)
Ah, logging call site: those are fairly limited, so I would not expect that to hit BigInteger ever :)
In the context of #354, using a global counter would also allow the change to also work for cljs
The likelihood of a ~64bit collision is small with the number of callsites, especially since the callsite id also incorporates other sources of entropy like the namespace, line, arg form, etc.
But a counter would indeed be simpler. This was a case of complexity growing organically over time :-) The initial choice of rand was quick+simple. Then the issue of determinism came up, then the GraalVM issue related to the determinism fix.
It was helpful of Michiel to re-check the original purpose: a counter is definitely the better choice now that we know we want determinism 👍
Thanks @ptaoussanis !
Closing this issue, but have opened #361 re: adding Graal tests.
Repro:
Compiling this gives:
This can be prevented by delaying the instantiating of the
java.util.Random
object to the first call of thedetermistic-rand
function.cc @DerGuteMoritz