Skip to content

Commit 2971331

Browse files
committed
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
1 parent e686d89 commit 2971331

4 files changed

Lines changed: 97 additions & 37 deletions

File tree

sjsonnet/src/sjsonnet/Importer.scala

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -307,15 +307,14 @@ class CachedResolver(
307307
val parsed: Either[Error, (Expr, FileScope)] = content.preParsedAst match {
308308
case Some(pre) => Right(pre)
309309
case None =>
310-
CachedResolver.parseJsonImport(
310+
val parsedJson = CachedResolver.parseJsonImport(
311311
path,
312312
content,
313313
internedStrings,
314314
settings
315-
) match {
316-
case Some(parsedJson) => Right(parsedJson)
317-
case None => parseJsonnet(path, content)
318-
}
315+
)
316+
if (parsedJson.isDefined) Right(parsedJson.get)
317+
else parseJsonnet(path, content)
319318
}
320319
parsed.flatMap { case (e, fs) => process(e, fs) }
321320
}
@@ -369,17 +368,17 @@ object CachedResolver {
369368
path: Path,
370369
content: ResolvedFile,
371370
internedStrings: mutable.HashMap[String, String],
372-
settings: Settings): Option[(Expr, FileScope)] = {
373-
if (!path.last.endsWith(".json")) return None
371+
settings: Settings): OptionVal[(Expr, FileScope)] = {
372+
if (!path.last.endsWith(".json")) return OptionVal.None
374373
val fileScope = new FileScope(path)
375374
try {
376375
val visitor =
377376
new JsonImportVisitor(fileScope, internedStrings, settings)
378-
Some((ujson.ByteArrayParser.transform(content.readRawBytes(), visitor), fileScope))
377+
OptionVal.some((ujson.ByteArrayParser.transform(content.readRawBytes(), visitor), fileScope))
379378
} catch {
380379
case _: ujson.ParsingFailedException | _: DuplicateJsonKey | _: InvalidJsonNumber |
381380
_: JsonParseDepthExceeded | _: NumberFormatException =>
382-
None
381+
OptionVal.None
383382
}
384383
}
385384

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package sjsonnet
2+
3+
/**
4+
* Optional reference value similar to `Option`, represented as a value class over a nullable
5+
* reference. Use this only in allocation-sensitive internal paths where the payload type is a
6+
* non-null reference value.
7+
*/
8+
private[sjsonnet] object OptionVal {
9+
@inline def some[A <: AnyRef](x: A): OptionVal[A] = {
10+
if (x eq null) throw new NullPointerException("OptionVal.some(null)")
11+
new OptionVal(x)
12+
}
13+
14+
/**
15+
* Boxed singleton sentinel; returning a freshly constructed null value class can erase to null.
16+
*/
17+
val None: OptionVal[Null] = new OptionVal[Null](null)
18+
}
19+
20+
private[sjsonnet] final class OptionVal[+A <: AnyRef](val x: A) extends AnyVal {
21+
@inline def isEmpty: Boolean = x eq null
22+
@inline def isDefined: Boolean = !isEmpty
23+
24+
@inline def get: A =
25+
if (isEmpty) throw new NoSuchElementException("OptionVal.None.get")
26+
else x
27+
28+
@inline def getOrElse[B >: A](default: => B): B =
29+
if (isEmpty) default else x
30+
}

sjsonnet/src/sjsonnet/Preloader.scala

Lines changed: 31 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -130,40 +130,43 @@ class Preloader(parentImporter: Importer, settings: Settings = Settings.default)
130130
}
131131

132132
private def discover(path: Path, content: ResolvedFile): Either[Error, Unit] = {
133-
CachedResolver.parseJsonImport(
133+
val parsedJson = CachedResolver.parseJsonImport(
134134
path,
135135
content,
136136
internedStrings,
137137
settings
138-
) match {
139-
case Some((expr, fs)) =>
140-
cache.put((path, false), PreParsedResolvedFile(content, expr, fs))
141-
Right(())
142-
case None =>
143-
val parser = new Parser(path, internedStrings, internedFieldSets, settings)
144-
try {
145-
fastparse.parse(content.getParserInput(), parser.document(_)) match {
146-
case f: Parsed.Failure =>
147-
val traced = f.trace()
148-
Left(new ParseError(s"$path: ${traced.msg}", offset = traced.index))
149-
case Parsed.Success((expr, fs), _) =>
150-
// Stash the parsed AST on the cache entry so the Interpreter doesn't re-run fastparse.
151-
// The optimizer still runs once at evaluation time on cache hit.
152-
cache.put((path, false), PreParsedResolvedFile(content, expr, fs))
153-
// Match the synchronous evaluator's docBase: resolve relative to the importing file's
154-
// parent directory, not the file path itself. See Importer.resolveAndReadOrFail, which
155-
// calls resolve(pos.fileScope.currentFile.parent(), ...).
156-
val docBase = path.parent()
157-
ImportFinder.collect(expr).foreach { found =>
158-
parentImporter.resolve(docBase, found.value).foreach { resolved =>
159-
record(resolved, found.kind)
160-
}
138+
)
139+
if (parsedJson.isDefined) {
140+
val parsed = parsedJson.get
141+
val expr = parsed._1
142+
val fs = parsed._2
143+
cache.put((path, false), PreParsedResolvedFile(content, expr, fs))
144+
Right(())
145+
} else {
146+
val parser = new Parser(path, internedStrings, internedFieldSets, settings)
147+
try {
148+
fastparse.parse(content.getParserInput(), parser.document(_)) match {
149+
case f: Parsed.Failure =>
150+
val traced = f.trace()
151+
Left(new ParseError(s"$path: ${traced.msg}", offset = traced.index))
152+
case Parsed.Success((expr, fs), _) =>
153+
// Stash the parsed AST on the cache entry so the Interpreter doesn't re-run fastparse.
154+
// The optimizer still runs once at evaluation time on cache hit.
155+
cache.put((path, false), PreParsedResolvedFile(content, expr, fs))
156+
// Match the synchronous evaluator's docBase: resolve relative to the importing file's
157+
// parent directory, not the file path itself. See Importer.resolveAndReadOrFail, which
158+
// calls resolve(pos.fileScope.currentFile.parent(), ...).
159+
val docBase = path.parent()
160+
ImportFinder.collect(expr).foreach { found =>
161+
parentImporter.resolve(docBase, found.value).foreach { resolved =>
162+
record(resolved, found.kind)
161163
}
162-
Right(())
163-
}
164-
} catch {
165-
case e: ParseError => Left(e)
164+
}
165+
Right(())
166166
}
167+
} catch {
168+
case e: ParseError => Left(e)
169+
}
167170
}
168171
}
169172

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package sjsonnet
2+
3+
import utest._
4+
5+
object OptionValTests extends TestSuite {
6+
def tests: Tests = Tests {
7+
test("some stores non-null references") {
8+
val value = new String("value")
9+
val option = OptionVal.some(value)
10+
assert(option.isDefined)
11+
assert(!option.isEmpty)
12+
assert(option.get eq value)
13+
assert(option.getOrElse("fallback") eq value)
14+
}
15+
16+
test("none widens to any reference payload type") {
17+
val option: OptionVal[String] = OptionVal.None
18+
assert(option.isEmpty)
19+
assert(!option.isDefined)
20+
assert(option.getOrElse("fallback") == "fallback")
21+
assertThrows[NoSuchElementException](option.get)
22+
}
23+
24+
test("some rejects null payloads") {
25+
assertThrows[NullPointerException](OptionVal.some(null))
26+
}
27+
}
28+
}

0 commit comments

Comments
 (0)