Namespace support in Compiler, currently relies in print-dup

[Thomas Heller]

Currently the compiler will generate

public static final Object const__2 = RT.readString("#=(find-ns shadow.build.warnings)");

If the code uses *ns*. It seems like an oversight that the compiler does not generate proper code for clojure.lang.Namespace. Relying on print-dup and readString seems terribly inefficient. I found this by accident and tools.logging is using *ns* every log call so every function that calls a log fn will have the above readString call.

Didn't want to go about writing a patch before confirming that this is actually useful. I think there should be a branch about here [1] that either directly calls Namespace.find or maybe a new helper fn in clojure.lang.RT (to avoid the extra import in the generated code).

Or is my instinct just wrong and the use of print-dup has a specific reason?

Cheer,

Thomas

[1] https://github.com/clojure/clojure/blob/9bca33911e130d3962dd75a13dbede9d49eea1f6/src/jvm/clojure/lang/Compiler.java#L4783

[Gary Fredericks]

When you said "terribly inefficient" and "every function that calls a log fn will have the above readString call" were you referring to a compile/load-time cost or a runtime cost?

Given the "static final" aspect of the attribute, I believe it's only a load-time cost.

In which case the severity is a question of how long it takes relative to the rest of compiling and loading.

Gary Fredericks

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev+unsubscribe@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at https://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Nicola Mometto]

It’s nor "terribly inefficient”, as, as Gary suspects, this is only executed at loading time, but it’s neither the case that this is emitted every time “the code uses *ns*” — that code path is only invoked if you *embed* objects in code, like namespace objects. i.e. if you do `(defmacro x [] *ns*) (defn curr-ns [] (x))`, but not if you do the more correct `(let [ns' *ns*] (defn curr-ns [] ns’))` instead

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.

[Gary Fredericks]

w.r.t. "more correct", I don't think this technique could work in the tools.logging case.

Gary Fredericks
(803)-295-0195
frederi...@gmail.com
gfredericks.com

[Thomas Heller]

On Tue, May 29, 2018 at 4:06 PM Gary Fredericks <frederi...@gmail.com> wrote:

When you said "terribly inefficient" and "every function that calls a log fn will have the above readString call" were you referring to a compile/load-time cost or a runtime cost?

Sorry, should have been more clear. load-time cost only. load-time matters though ...

A rough estimate in my code is that I have about 100 clojure.tools.logging usages. Assuming they are all in independent fns each is has its own .class generated. That is 100 times RT.readString during loading for my lib (shadow-cljs, experimenting with AOT). I have not benchmarked this but I'm willing to wager that this is at least one order of magnitude slower, probably more, than just calling Namespace.find.

It is a minor optimization, but since it affects load time it seems worth it.

[Nicola Mometto]

Even then, it’s absolutely negligible

user=> (quick-bench (read-string "#=(find-ns user)"))

Evaluation count : 226824 in 6 samples of 37804 calls.

             Execution time mean : 2.019092 µs

    Execution time std-deviation : 99.951478 ns

   Execution time lower quantile : 1.899425 µs ( 2.5%)

   Execution time upper quantile : 2.120700 µs (97.5%)

                   Overhead used : 2.399639 ns

nil

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.

[Gary Fredericks]

I just tried something similar on a project with 189 namespaces loaded, and it did 1000 of the read-string calls in <10ms.

Gary Fredericks
(803)-295-0195
frederi...@gmail.com
gfredericks.com

On Tue, May 29, 2018 at 9:36 AM, Nicola Mometto <brob...@gmail.com> wrote:

Even then, it’s absolutely negligible

user=> (quick-bench (read-string "#=(find-ns user)"))

Evaluation count : 226824 in 6 samples of 37804 calls.

             Execution time mean : 2.019092 µs

    Execution time std-deviation : 99.951478 ns

   Execution time lower quantile : 1.899425 µs ( 2.5%)

   Execution time upper quantile : 2.120700 µs (97.5%)

                   Overhead used : 2.399639 ns

nil

On 29 May 2018, at 15:31, Thomas Heller <in...@zilence.net> wrote:

On Tue, May 29, 2018 at 4:06 PM Gary Fredericks <frederi...@gmail.com> wrote:

When you said "terribly inefficient" and "every function that calls a log fn will have the above readString call" were you referring to a compile/load-time cost or a runtime cost?

Sorry, should have been more clear. load-time cost only. load-time matters though ...

A rough estimate in my code is that I have about 100 clojure.tools.logging usages. Assuming they are all in independent fns each is has its own .class generated. That is 100 times RT.readString during loading for my lib (shadow-cljs, experimenting with AOT). I have not benchmarked this but I'm willing to wager that this is at least one order of magnitude slower, probably more, than just calling Namespace.find.

It is a minor optimization, but since it affects load time it seems worth it.

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev+unsubscribe@googlegroups.com.

To post to this group, send email to cloju...@googlegroups.com.
Visit this group at https://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev+unsubscribe@googlegroups.com.

[Thomas Heller]

10000 is a bit excessive but you get my point ...

(ns shadow.benchmark
  (:import [clojure.lang RT Namespace]))

(time
  (dotimes [i 10000]
    (RT/readString "#=(find-ns shadow.benchmark)")))

(time
  (dotimes [i 10000]
    (Namespace/find 'shadow.benchmark)
    ))

"Elapsed time: 17.596967 msecs"
"Elapsed time: 0.306979 msecs"

I assume that quick-bench does a bunch of warmup for the JIT and other stuff you want in actual benchmarks. Load-time is different.

[Nicola Mometto]

That time is being spent by `find-ns` not by the read-string call anyway, which is doing _very little work_ and `find-ns` is work that you’d need to do even if you chose another serialisation strategy. I really don’t think this is a problem.. 

[Nicola Mometto]

I misread your benchmark — OK I see what you mean but the difference is still negligible IMO — In realistic terms, those are 17ms spent loading 10_000 vars, a process that would likely take at least 2/3 seconds (being really generous and assuming massively simple vars)

[Thomas Heller]

Again though: Load-Time is different. The JIT is not warm and so the numbers WILL look different there. I don't know how to benchmark this properly in a way that would reflect the actual situation.

Yes the impact is probably minimal. Never said this was a problem. Found something that looked inefficient and could be made more efficient with about 5 lines of code. Even if it just 10ms out of 2000ms I would call that a win.