perf(kotlin): resolve operand paths lazily, and fix numeric comparison - #56
Merged
Conversation
Operand paths are now resolved on demand against the input, visiting only the nodes a path names, instead of eagerly flattening the entire input into a map keyed by dotted paths. Cost scales with the rule, not the input width. InputPath handles the resolution: maps by key (or `toString()` for non-string keys), collections and arrays by index, and plain objects by cached Kotlin reflection. A path that does not exist (missing key, out-of-range index, or traversal past a value) throws, which `onFailure` turns into a rule result, preserving the original semantics. Benchmarks show ~2.7x throughput gain on the default input (2.1M → 0.78M ops/s before this, now 2.1M again), no cost for wide inputs (previously degraded by 11.7x), and elimination of the classloader pinning from the unbounded property cache (now scoped to the engine instance).
A list written in an expression holds operands, but only String operands were resolved, so a quoted element kept its quotes and never matched the resolved right operand. Elements keep their own type: the Number to BigDecimal normalization stays on the operand as a whole, matching lists read from the input data.
Measures bytes allocated per evaluation and garbage collection counts during benchmark runs, using HotSpot ThreadMXBean for thread-local allocation and ManagementFactory for JVM-wide GC stats. Also adds standard deviation to latency distribution reporting. Allocation metrics reveal whether engine overhead is bound by memory allocation or execution cost. Current results show allocation ranging from 866 B per evaluation on Kotlin to 128 KB on default GraalJS.
Numbers now compare by value using BigDecimal.compareTo instead of equals, so 10 and 10.00 are the same number and fractions are never truncated. All numbers normalize to BigDecimal, including elements in collections, so listOf(1, 2) expContains 1 matches. Updated contains operator to compare elements by value. Added NumberCases test suite and updated benchmark results to reflect new test count (173 expressions, up from 150).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the Kotlin engine's per-evaluation input flattening with lazy path resolution, then fixes two numeric
correctness bugs the new test coverage exposed. Adds allocation and GC tracking to the benchmark harness, plus a wide
input variant that separates a cost scaling with the input from one scaling with the rule.
Lazy path resolution
KotlinEvaluator.callwalked the whole input graph on everyevaluateand materialised a flat map of every path, soa rule reading one field of a wide object paid for every other field and every element of every list.
InputPath.resolve(root, path)now walks the input on demand, visiting only the nodes a path names. The path isscanned in place, so
a.b[0].callocates oneStringper named segment and nothing else.memberPropertiesisreflected once per class into a cache that also serves the name lookup.
The absence rules were read off the old
parseKeysand reproduced exactly, because they decide what throws andtherefore what
onFailureswallows:""Collectionor anArraynameunder aMapcontainsKey(name), or a key whosetoString()matchesnameunder an objectjavaClass.kotlin.memberPropertiesreports itnameunder a value, anull, aCollectionor anArray[i]CollectionorArrayandiis in range, by iteration orderKotlinPathResolutionTestcovers the table.Three differences from the flattened implementation, none covered by an existing test:
KotlinContext's constructor takesAnyinstead ofMap<String, Any?>. Source-compatible for callers passing amap, binary-incompatible.
Mapkey containing.or[was a literal key in the flat map and is read as a path now.evaluateof every rule, because flattening touchedevery property. It fails only when a rule names it now.
Numeric fixes
BigDecimal.valueOf(this.toLong())truncated everyBigDecimaloperand to its integer part, on both sides of acomparison, and overflowed above
Long.MAX_VALUE:A
BigDecimalis now kept as it is and aBigIntegeris converted directly. Equality compares numbers by valuerather than by representation, so
10and10.00are the same number, which comparisons already did throughcompareTo. Normalization also reaches into collections, guarded so a list without numbers is not rebuilt, whichmakes
listOf(1, 2) expContains 1match.The Rhino engine diverged on the same case:
Containsdelegated toindexOf, which on ajava.util.Listis Javaequality, so a JS number never matched a boxed
Integer. It scans with==now, and throws for a non-container soonFailurebehaves as it does on the Kotlin engine.NumberCasescovers fractions, scale and numeric lists, and runs against all three engines throughBaseEvaluatorTest.Benchmark harness
alloc/op, total allocation, GC collections and time, and the standard deviation of an iteration are reported perrun. Allocation comes from the JVM per-thread counter and reports
n/awhere it is unavailable.-PbenchWide=Nruns the same rules against the same fields under a root carryingNextra scalar fields and anNelement list. Wired into all three
benchtasks, which now take the same arguments in the same order.BaseEvaluatorTest.assertWideInputMatchesDefaultInputasserts every case gives the same result against both inputson every engine, so the variant measures the same work.
Measured
2000 iterations of the 173 expression suite, Apple M3 Pro with Amazon Corretto 21.0.11, three runs per configuration
in one session, medians:
Against the flattening implementation on its own suite, lazy resolution measured 2.1x on the benchmark input and
23.8x on a wide one, where widening the input cost the flattened version 11.7x with the rules unchanged.
Input width, same session:
Both JS engines scale with input width: they inject every top-level entry of the input into the scope on every
evaluate, and the allocation factor tracks the throughput factor on both. Neither scales with depth. Width is thelargest per-evaluation cost left on reused-context GraalJS.
Test plan
./gradlew testgreen on all three engines, with the existing cases unmodified./gradlew detekt koverVerifygreen./gradlew :kotlin-evaluator:bench -PbenchIterations=2000and the same with-PbenchWide=200