Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A more efficient alternative syntax #112

Closed
igorgatis opened this issue Apr 24, 2020 · 4 comments
Closed

A more efficient alternative syntax #112

igorgatis opened this issue Apr 24, 2020 · 4 comments

Comments

@igorgatis
Copy link

igorgatis commented Apr 24, 2020

I'd like to propose a syntax which is more efficient than current implementation.

Current Syntax

logger.debug { "Some $expensive message!" }

Is great because it becomes somewhat:

if (logger.isDebugEnabled) logger.debug("Some $expensive message!")

Decompiled code for current syntax

The truth is that using a decompiler (such as Jad), code produced looks like this:

// logger.debug { "Some $expensive message!" }
Companion.getLogger().debug((Function0)new Function0(expensive) {
  final String $expensive;
  public Function0(string expensive) {
    $expensive = expensive;
  }
  public volatile Object invoke() {
    return invoke();
  }
  public final String invoke() {
    return new StringBuilder()
       .append("Some ").append($expensive).append(" message!")
       .toString();
  }
});

Therefore, even when debug level is disabled, here are the penalties:

  1. Code size: as it adds an annonymous class that extends kotlin.jvm.functions.Function0 which makes final code bigger.
  2. Memory: as it allocates memory for the annonymous class instance although debug() will ignore it.
  3. CPU: as it assigns valirables to members of annonymous class and also needs to GC the ignored instance.

Proposed Alternative New Syntax

Now compare with the following syntax:

logger.debug?.println("Some $expensive message!")

Where logger is of type:

interface LogWriter {
  fun println(line: String)
}

class AltLogger() {
  companion object {
    private logLevel: int = // resolved somehow.
  }

  val debug: LogWriter? = if (logLevel >= 3) LogWriterImpl() else null
  val info: LogWriter? = if (logLevel >= 2) LogWriterImpl() else null
  val warn: LogWriter? = if (logLevel >= 1) LogWriterImpl() else null
  val error: LogWriter? = if (logLevel >= 0) LogWriterImpl() else null
}

Decompiled code for proposed syntax

Decompiled code will look like this:

// logger.debug?.println("Some $expensive message!")
if (Companion.getLogger() != null) {
  Companion.getLogger().println(
    new StringBuilder()
       .append("Some ").append(expensive).append(" message!")
       .toString()
  );
}

Notice that nothing is allocated or assigned. Simple plain check against null before any cost.

@oshai
Copy link
Owner

oshai commented Apr 26, 2020

I really like the idea. However changing the syntax might break compatibility in a way that might be really inconvenient to migrate. In addition, issue #34 also discuss approaches to reduce the overhead.
I think unless we have a lot of people supporting such approach I will be hesitant to change the lib.

@dnut
Copy link

dnut commented May 18, 2020

I don't like println. I would use invoke.

logger.debug?("Some $expensive message!")

I'm not convinced this is better than using inline functions. Actually I think it's worse because you're relying on a specific compiler implementation. Is this how kotlin/native works? What if future versions of kotlin implement this differently? We would need a contract that kotlin will definitely never evaluate the string eagerly. We get that with a lambda.

@oshai
Copy link
Owner

oshai commented Aug 18, 2020

Closing the issue as it doesn't seem we're going to do that change ATM.

@oshai oshai closed this as completed Aug 18, 2020
@sghpjuikit
Copy link

sghpjuikit commented Aug 20, 2020

@dnut
invoke operator can not be invoked on nullable types

logger.debug?("Some $expensive message!")

One needs to use this syntax instead

logger.debug?.invoke("Some $expensive message!")

I think leveraging ?. is super clever. ++. And I like the idiomatic use of null for logging level check. But the invoke part (more precisely the need for another method at the log call site) is red flag.

I do not think we are relying on some internal magic here. ?. and ?: are similar to Java's ternary operator, which is also lazy. These can not become eager - they are lazy by intention - changing this would alter semantics of many programs in a dramatic way. I feel this behavior is reliable - https://kotlinlang.org/docs/kotlin-docs.pdf specifically says ?: is lazy, although, not ?.. I do admit though that I did not expect ?. to be lazy enough to not even evaluate parameters.

In the end, I'm also against this solution. The reason is simple: the lazy nature of the ?. is counter-intuitive, particularly when compared to the inline {} (block syntax), which is idiomatic representation of 'lazy' things, which is only natural evolution of how Java worked as well.

On the other hand ?. and ?: are too magical. The fantastical expression logger.debug?("Some $expensive message!") perfectly shows why - there is argument expression, which is unintuitively not being evaluated (depending on the log level). It IS unintuitive, no matter how much Scala guys would claim otherwise. This is literally what Scala allows to do with 'call-by-name' Logger.debug(message: =>String) - call the method normally, but get full lazyness - and that is very BAD thing, because the argument evaluation semantics changes depending on the method. Too much safety is lost.

Yes, logging is actually an ideal scenario where call-by-name is a great fit - it leads to the simplest API and easiest use. But even then I consider call-by-name hideous and counter-productive mechanism. Yes, it gives immense power (in Scala it is possible to create ?. as an ordinary method!), but the presence of multiple argument evaluation strategies in the language is not mundane complication. I think lazy should always be explicit, always, and Kotlin's inline block syntax is exactly how things should be.

PS: Un/fortunately, without call-by-name, logging in Kotlin will always feel +- hacky as every solution will be an attempt to simulate call-by-name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants