[prelude][core] Frame and Log optimizations #800
Conversation
| @@ -25,34 +25,34 @@ object LogPlatformSpecific: | |||
| else Level.error | |||
|
|
|||
| inline def trace(msg: => String)(using frame: Frame, allow: AllowUnsafe): Unit = | |||
| if Level.trace.enabled(level) then logger.trace(s"[${frame.parse.position}] $msg") | |||
There was a problem hiding this comment.
The parse method was quite expensive since it parsed all the fields, allocating several sub-strings. I've optimized Frame to provide more lightweight methods to access the information it contains.
| @@ -2,7 +2,7 @@ package kyo | |||
|
|
|||
| import kyo.internal.LogPlatformSpecific | |||
|
|
|||
| final case class Log(unsafe: Log.Unsafe) extends AnyVal: | |||
| final case class Log(unsafe: Log.Unsafe): | |||
There was a problem hiding this comment.
I noticed several Log allocations in the benchmark. There are scenarios where both the compiler and the JIT fail to prevent allocation, triggering the boxing every time Log is used. By removing AnyVal, the Log is always allocated once when created and then reused. I'll take a look at making this change for the other effects as well.
| @@ -67,18 +67,19 @@ object Ansi: | |||
| */ | |||
| def apply(header: String, code: String, trailer: String, startLine: Int = 1): String = | |||
| try | |||
| val headerLines = if header.nonEmpty then header.split("\n") else Array.empty[String] | |||
There was a problem hiding this comment.
A few cosmetic improvements
fwbrasil
force-pushed
the
frame-log-opt
branch
from
9f72f70 to
9017432
Compare
| frame.parse.show, | ||
| "──────────────────────────────".dim | ||
| ).mkString("\n") | ||
| override def getMessage() = frame.render("Check failed! ".red.bold + message) |
There was a problem hiding this comment.
New convenience frame.render for this kind of use case.
| @@ -23,8 +23,8 @@ abstract class Local[A]: | |||
| * @return | |||
| * An effect that produces the current value | |||
| */ | |||
| final def get(using Frame): A < Any = | |||
There was a problem hiding this comment.
These weren't inlined I believe because of compilation issues since we typically inline suspension methods. I've made State public and was able to inline the methods.
| if end < 0 then self.substring(start) | ||
| else self.substring(start, end) | ||
|
|
||
| def position: Position = self |
There was a problem hiding this comment.
Note that there's no allocation when frame.position is called. It's just a wrapper that provides methods to read the specific fields.
| self.substring(start) | ||
| end snippet | ||
|
|
||
| def snippetShort: String = snippet.split("📍")(0).reverse.take(snippetShortMaxChars).takeWhile(_ != '\n').trim.reverse |
There was a problem hiding this comment.
this expensive string transformation was being done on each benchmark iteration 😱
| @@ -9,45 +9,88 @@ opaque type Frame <: AnyRef = String | |||
|
|
|||
| object Frame: | |||
|
|
|||
| private val version = '0' | |||
There was a problem hiding this comment.
I've added a version field as the first character of the Frame payload so we can more easily make changes to the format in the future.
| @@ -17,42 +17,42 @@ object LogPlatformSpecific: | |||
| def apply(name: String) = new SLF4J(org.slf4j.LoggerFactory.getLogger(name)) | |||
|
|
|||
| class SLF4J(logger: org.slf4j.Logger) extends Log.Unsafe: | |||
| def level = | |||
There was a problem hiding this comment.
The main bug was here. The logic was inverted.
There was a problem hiding this comment.
I don't think this can be a val - it will be transient for an slf4j logger based on ThreadLocal? Not that I rely on that feature...
There was a problem hiding this comment.
I think it'd be an issue only if the SLF4J config dynamically changes at runtime. I've never used such a feature, I'm not sure if it's possible or how useful it'd be. I made it a val because, in theory, we'd need an IO suspension to read the level otherwise.
fwbrasil
force-pushed
the
frame-log-opt
branch
from
9017432 to
64f3055
Compare
| @@ -17,42 +17,42 @@ object LogPlatformSpecific: | |||
| def apply(name: String) = new SLF4J(org.slf4j.LoggerFactory.getLogger(name)) | |||
|
|
|||
| class SLF4J(logger: org.slf4j.Logger) extends Log.Unsafe: | |||
| def level = | |||
There was a problem hiding this comment.
I don't think this can be a val - it will be transient for an slf4j logger based on ThreadLocal? Not that I rely on that feature...
| val trailerLines = if trailer.nonEmpty then trailer.split("\n") else Array.empty[String] | ||
| val separatorLine = "─".repeat(30).dim | ||
| val headerLines = if header.nonEmpty then Array(separatorLine) ++ header.split("\n") else Array.empty[String] | ||
| val codeLines = code.split("\n").dropWhile(_.trim.isEmpty).reverse.dropWhile(_.trim.isEmpty).reverse |
There was a problem hiding this comment.
.isBlank should avoid new string allocations.
Following up on #800 (comment). Since Scala 3 doesn't have specialization `AnyVal` values can keep boxing on every transformation.
I noticed a major performance regression in
LoggingBench. The main cause is a bug introduced by #783 that makes the benchmark considerLevel.errorenabled when it isn't. I fixed the bug but also worked on a few optimizations to improve performance in case the log is enabled.Benchmark results: