From 2971331e444af29ed996889b4bbdd5203fcba119 Mon Sep 17 00:00:00 2001 From: He-Pin Date: Sun, 5 Jul 2026 16:39:02 +0800 Subject: [PATCH] refactor: add OptionVal for JSON import fast path Motivation: Strict .json imports return an optional parse result so callers can fall back to the Jsonnet parser. The previous Option[(Expr, FileScope)] result allocated a scala.Some on each successful JSON fast-path parse. Modification: Add a small internal OptionVal value class for optional non-null reference values and use it for CachedResolver.parseJsonImport and the Preloader call site. Reject null payloads, add direct OptionVal regression tests, and avoid tuple-pattern destructuring in the preloader fast path so the generated bytecode does not introduce an extra Tuple2 allocation. Result: Successful strict .json imports no longer allocate a scala.Some wrapper while preserving fallback behavior for non-JSON or invalid JSON inputs across evaluator and preloader paths. OptionVal's core semantics are covered by tests, and the preloader JSON fast path keeps the intended lower-allocation shape. References: https://github.com/apache/pekko/blob/main/actor/src/main/scala/org/apache/pekko/util/OptionVal.scala --- sjsonnet/src/sjsonnet/Importer.scala | 17 +++--- sjsonnet/src/sjsonnet/OptionVal.scala | 30 ++++++++++ sjsonnet/src/sjsonnet/Preloader.scala | 59 ++++++++++--------- .../test/src/sjsonnet/OptionValTests.scala | 28 +++++++++ 4 files changed, 97 insertions(+), 37 deletions(-) create mode 100644 sjsonnet/src/sjsonnet/OptionVal.scala create mode 100644 sjsonnet/test/src/sjsonnet/OptionValTests.scala diff --git a/sjsonnet/src/sjsonnet/Importer.scala b/sjsonnet/src/sjsonnet/Importer.scala index f5a2257d5..7256b521f 100644 --- a/sjsonnet/src/sjsonnet/Importer.scala +++ b/sjsonnet/src/sjsonnet/Importer.scala @@ -307,15 +307,14 @@ class CachedResolver( val parsed: Either[Error, (Expr, FileScope)] = content.preParsedAst match { case Some(pre) => Right(pre) case None => - CachedResolver.parseJsonImport( + val parsedJson = CachedResolver.parseJsonImport( path, content, internedStrings, settings - ) match { - case Some(parsedJson) => Right(parsedJson) - case None => parseJsonnet(path, content) - } + ) + if (parsedJson.isDefined) Right(parsedJson.get) + else parseJsonnet(path, content) } parsed.flatMap { case (e, fs) => process(e, fs) } } @@ -369,17 +368,17 @@ object CachedResolver { path: Path, content: ResolvedFile, internedStrings: mutable.HashMap[String, String], - settings: Settings): Option[(Expr, FileScope)] = { - if (!path.last.endsWith(".json")) return None + settings: Settings): OptionVal[(Expr, FileScope)] = { + if (!path.last.endsWith(".json")) return OptionVal.None val fileScope = new FileScope(path) try { val visitor = new JsonImportVisitor(fileScope, internedStrings, settings) - Some((ujson.ByteArrayParser.transform(content.readRawBytes(), visitor), fileScope)) + OptionVal.some((ujson.ByteArrayParser.transform(content.readRawBytes(), visitor), fileScope)) } catch { case _: ujson.ParsingFailedException | _: DuplicateJsonKey | _: InvalidJsonNumber | _: JsonParseDepthExceeded | _: NumberFormatException => - None + OptionVal.None } } diff --git a/sjsonnet/src/sjsonnet/OptionVal.scala b/sjsonnet/src/sjsonnet/OptionVal.scala new file mode 100644 index 000000000..52e76e655 --- /dev/null +++ b/sjsonnet/src/sjsonnet/OptionVal.scala @@ -0,0 +1,30 @@ +package sjsonnet + +/** + * Optional reference value similar to `Option`, represented as a value class over a nullable + * reference. Use this only in allocation-sensitive internal paths where the payload type is a + * non-null reference value. + */ +private[sjsonnet] object OptionVal { + @inline def some[A <: AnyRef](x: A): OptionVal[A] = { + if (x eq null) throw new NullPointerException("OptionVal.some(null)") + new OptionVal(x) + } + + /** + * Boxed singleton sentinel; returning a freshly constructed null value class can erase to null. + */ + val None: OptionVal[Null] = new OptionVal[Null](null) +} + +private[sjsonnet] final class OptionVal[+A <: AnyRef](val x: A) extends AnyVal { + @inline def isEmpty: Boolean = x eq null + @inline def isDefined: Boolean = !isEmpty + + @inline def get: A = + if (isEmpty) throw new NoSuchElementException("OptionVal.None.get") + else x + + @inline def getOrElse[B >: A](default: => B): B = + if (isEmpty) default else x +} diff --git a/sjsonnet/src/sjsonnet/Preloader.scala b/sjsonnet/src/sjsonnet/Preloader.scala index 2c5a483d1..eb87a0530 100644 --- a/sjsonnet/src/sjsonnet/Preloader.scala +++ b/sjsonnet/src/sjsonnet/Preloader.scala @@ -130,40 +130,43 @@ class Preloader(parentImporter: Importer, settings: Settings = Settings.default) } private def discover(path: Path, content: ResolvedFile): Either[Error, Unit] = { - CachedResolver.parseJsonImport( + val parsedJson = CachedResolver.parseJsonImport( path, content, internedStrings, settings - ) match { - case Some((expr, fs)) => - cache.put((path, false), PreParsedResolvedFile(content, expr, fs)) - Right(()) - case None => - val parser = new Parser(path, internedStrings, internedFieldSets, settings) - try { - fastparse.parse(content.getParserInput(), parser.document(_)) match { - case f: Parsed.Failure => - val traced = f.trace() - Left(new ParseError(s"$path: ${traced.msg}", offset = traced.index)) - case Parsed.Success((expr, fs), _) => - // Stash the parsed AST on the cache entry so the Interpreter doesn't re-run fastparse. - // The optimizer still runs once at evaluation time on cache hit. - cache.put((path, false), PreParsedResolvedFile(content, expr, fs)) - // Match the synchronous evaluator's docBase: resolve relative to the importing file's - // parent directory, not the file path itself. See Importer.resolveAndReadOrFail, which - // calls resolve(pos.fileScope.currentFile.parent(), ...). - val docBase = path.parent() - ImportFinder.collect(expr).foreach { found => - parentImporter.resolve(docBase, found.value).foreach { resolved => - record(resolved, found.kind) - } + ) + if (parsedJson.isDefined) { + val parsed = parsedJson.get + val expr = parsed._1 + val fs = parsed._2 + cache.put((path, false), PreParsedResolvedFile(content, expr, fs)) + Right(()) + } else { + val parser = new Parser(path, internedStrings, internedFieldSets, settings) + try { + fastparse.parse(content.getParserInput(), parser.document(_)) match { + case f: Parsed.Failure => + val traced = f.trace() + Left(new ParseError(s"$path: ${traced.msg}", offset = traced.index)) + case Parsed.Success((expr, fs), _) => + // Stash the parsed AST on the cache entry so the Interpreter doesn't re-run fastparse. + // The optimizer still runs once at evaluation time on cache hit. + cache.put((path, false), PreParsedResolvedFile(content, expr, fs)) + // Match the synchronous evaluator's docBase: resolve relative to the importing file's + // parent directory, not the file path itself. See Importer.resolveAndReadOrFail, which + // calls resolve(pos.fileScope.currentFile.parent(), ...). + val docBase = path.parent() + ImportFinder.collect(expr).foreach { found => + parentImporter.resolve(docBase, found.value).foreach { resolved => + record(resolved, found.kind) } - Right(()) - } - } catch { - case e: ParseError => Left(e) + } + Right(()) } + } catch { + case e: ParseError => Left(e) + } } } diff --git a/sjsonnet/test/src/sjsonnet/OptionValTests.scala b/sjsonnet/test/src/sjsonnet/OptionValTests.scala new file mode 100644 index 000000000..8bd938287 --- /dev/null +++ b/sjsonnet/test/src/sjsonnet/OptionValTests.scala @@ -0,0 +1,28 @@ +package sjsonnet + +import utest._ + +object OptionValTests extends TestSuite { + def tests: Tests = Tests { + test("some stores non-null references") { + val value = new String("value") + val option = OptionVal.some(value) + assert(option.isDefined) + assert(!option.isEmpty) + assert(option.get eq value) + assert(option.getOrElse("fallback") eq value) + } + + test("none widens to any reference payload type") { + val option: OptionVal[String] = OptionVal.None + assert(option.isEmpty) + assert(!option.isDefined) + assert(option.getOrElse("fallback") == "fallback") + assertThrows[NoSuchElementException](option.get) + } + + test("some rejects null payloads") { + assertThrows[NullPointerException](OptionVal.some(null)) + } + } +}