<< Back to previous view

[TLOG-15] Generated logging code uses eval to get logger namespace Created: 02/Apr/15  Updated: 05/Apr/15

Status: Open
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Alexander Yakushev Assignee: Alexander Yakushev
Resolution: Unresolved Votes: 0
Labels: None

Java 1.7.0_75, org.clojure/clojure 1.7.0-alpha6, org.clojure/tools.logging 0.3.1

Attachments: File TLOG-15-patch.diff    
Patch: Code


Logging macros that use current namespace generate incorrect code that relies on eval.

Example code:

(defn divide [x y]
  (log/info "dividing" x "by" y))

From decompiled Java class:

    public static final Object const__2 = RT.readString("#=(find-ns logtest.core)");

const_2 is being assigned a value read from a string within the evaluation reader macro. This is an issue in environments where dynamic compilation is not available, and in any case having code that is evaluated in runtime is sub-optimal.

The attached patch solves this problem by turning namespace into a string early, so that it can be properly serialized as a string by the macros.

Comment by Alexander Taggart [ 05/Apr/15 12:12 PM ]

The logger-ns is used by an underlying implementation to decide which logger instance to yield. While commonly a string (e.g., in SLF4J), there is nothing in the tools.logging API that requires the logger-ns to be a string. For example, one could create an implementation of LoggerFactory that uses logger-ns's metadata to convey configuration to a logger instance.

All that said, this issue looks like an artifact of the compiler. I'm not familiar enough with the particulars of your environment, but if there is a way to improve the code without changing behaviour, I would welcome it. Barring that, my sense is this may best be resolved via improvements to the clojure compiler rather than this particular lib.

Leaving this ticket open pending feedback.

Comment by Alexander Yakushev [ 05/Apr/15 12:46 PM ]

Thank you for the response. I've opened this issue before I discovered that tools.logging.impl entirely relies on eval, so I made just a quick hack.

The original behavior is not an artifact of the compiler, but compiler's response to trying to serialize namespace instance in code. Because namespace objects are not actually serializable, the compiler circumvents this limitation by inserting this eval-dependent call that will resolve to the namespace (if the latter is still around).

What can be done, though, is making logger-ns a map that contains all namespace's metadata plus its name, of course. And then the implementations can decide do they just get :name from the map, or consider other data as well.

Comment by Alexander Taggart [ 05/Apr/15 1:25 PM ]

I think I see what's going on. Right now, the log macro arity that uses ns passes the realized namespace object to another arity, thus the expanded code that calls impl/get-logger holds a reference to the specific namespace object rather than just literally using ns.

One way to eliminate a ns read-eval would be to have the expanded code only refer to ns. That said, it's not clear to me whether there would be any performance improvement; I suspect it might actually be a performance hit, namely getting the value of a dynamic var every invocation vs the constant's one-time cost.

I want to make as few assumptions as possible about what values people are passing, since that would constrain future backend implementations. As it stands, the only assumption about logger-ns is that the provided java impl factories expect (str logger-ns) to work.

Comment by Alexander Yakushev [ 05/Apr/15 1:32 PM ]

My problem with re-evaled namespace was not about the performance, but about the availability of eval. And this is/will be an issue in ClojureScript too. Still, as we concluded in #14, it is easier for me just to keep my fork until the reader conditionals stabilize.

Comment by Alexander Taggart [ 05/Apr/15 1:38 PM ]

Got it. Out of curiosity, can you try editing the 3-arity method of the log macro by changing ~ns to just ns, and see how it behaves? I suspect this is what it should have been all along.

[TLOG-14] Logging backend (factory) for Android Created: 02/Apr/15  Updated: 05/Apr/15

Status: Open
Project: tools.logging
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alexander Yakushev Assignee: Alexander Yakushev
Resolution: Unresolved Votes: 0
Labels: android, enhancement



I would like to use tools.logging on Android. Android OS already provides a logging backend (in the form of android.util.Log class), so it is enough to write a factory for it in tools.logging.impl. I would like to write this factory if the maintainers agree to accept the patch.

The created factory would not strongly depend upon any Android classes, it will use Class/forName to check for Android environment. About the position of this factory between others: I think it is best to put it just before java.util.logging, so that more specific implementations are preferred.

Comment by Alexander Taggart [ 05/Apr/15 11:40 AM ]

Would it be reasonable to leverage SLF4J here? See http://www.slf4j.org/android/

Comment by Alexander Yakushev [ 05/Apr/15 12:14 PM ]

While it is indeed possible to do go this route, I just didn't want to pull in another wrapper (slf4j-android) in addition to clojure.tools.logging (which is also a wrapper). Could be matter of taste though.

On a related note, I still had to rewrite all existing factories in clojure.tools.logging.impl. They all use eval which is not available in the limited Android environment, so I turned them into macros that expand only if the respective classes are on the classpath at AOT time. Have you considered adopting similar behavior for mainline tools.logging, or is there specific requirement to keep eval?

Comment by Alexander Taggart [ 05/Apr/15 12:46 PM ]

The multiple wrapping used to bug me too, but I figured they're providing a different set of features. Really, tools.logging just provides some clojure-specific behaviour (e.g., conditional evaluation via macros, binding out and err, using ns by default), whereas SLF4J is proving a common java API, which in turn means I don't need to maintain an ever-growing list of java logging implementations .

As for the use of eval, ironically, it was added precisely to avoid the logging implementation becoming fixed at the time of performing AOT. This makes sense if one is, say, AOTing a generic library. Absence of the ability to eval was not foreseen.

It might be possible to conditionally skip the eval when eval is not present, though ad hoc feature detection isn't something I'd want to do without significant input from the community. Perhaps a better route would be to use 1.7's reader conditionals, though I only see platform-based distinctions between clj/cljs/cljr; nothing suggesting a way to vary based on android.

Comment by Alexander Yakushev [ 05/Apr/15 12:55 PM ]

I see. I thought the case for eval there is rather marginal (if you AOT compile tools.logging without the logging implementation) but apparently someone needs that. I also do AOT compilation but I compile everything together and at once, so macro approach works for me.

Perhaps then I shouldn't bother you with this, and just maintain my fork for Android projects. It's too much work, at least for now, to cater to all possible platforms in a single version.

Comment by Alexander Taggart [ 05/Apr/15 1:05 PM ]

If the android platform has enough significant differences from vanilla clj, then I would think that's precisely the purpose of 1.7's conditional reader support. I suggest reaching out to the dev mailing list.

Generated at Mon Nov 30 14:42:52 CST 2015 using JIRA 4.4#649-r158309.