perf: promote hot-path evaluator to default; remove dual-evaluator flag#788
Open
He-Pin wants to merge 1 commit intodatabricks:masterfrom
Open
perf: promote hot-path evaluator to default; remove dual-evaluator flag#788He-Pin wants to merge 1 commit intodatabricks:masterfrom
He-Pin wants to merge 1 commit intodatabricks:masterfrom
Conversation
… flag Motivation: The hybrid instanceof + tableswitch dispatch introduced as NewEvaluator (databricks#785) was opt-in via --new-evaluator / Settings.useNewEvaluator. Maintaining two evaluators doubles the surface for every future optimization, splits test runs (each EvaluatorTests case ran twice), and leaves the slow path as the default for users who don't know to flip the flag. JMH on the existing 18 EvaluatorBenchmark files (1 fork x 5 warmup x 3s + 10 measure x 3s; bench.06 re-verified with 3 forks x 8 warmup + 15 measure) shows the new dispatch is statistically equal-or-better everywhere, with geomean -7.27% and up to -37% on bench.03. The single 1-fork outlier (bench.06 +4.03%) reproduced as a tie on 3-fork rerun (0.186 vs 0.186 ms). Modification: - Lift NewEvaluator.visitExpr (7-type instanceof hot path + tag-switch cold path) into base Evaluator. Profiler instrumentation is preserved in the hot path -- previously NewEvaluator silently dropped it. - Delete NewEvaluator class. - Remove Settings.useNewEvaluator and the --new-evaluator CLI flag (breaking change for any embedder that set the flag explicitly; the flag was the slower path, so removing it improves their default). - Collapse EvaluatorTests.allTests(useNewEvaluator) into a single tests block (no more 2x prefixed run). - Remove the now-redundant EvaluatorBenchmark A/B harness; ongoing perf is covered by MainBenchmark and RegressionBenchmark. Result: One evaluator implementation. All 12 JVM Scala 3 test suites pass with 0 failures. -260 net lines of code.
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
1 participant
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
Lifts the
NewEvaluatorhot-path dispatch (introduced in #785 behind--new-evaluator) into the defaultEvaluator, then removes the now-redundant subclass,Settings.useNewEvaluator, and the--new-evaluatorCLI flag. Maintaining two evaluators doubled the surface for every future optimization and left the slow path as the user-facing default.Benchmarks
JMH (
EvaluatorBenchmark, 18 files × 1 fork × 5 warmup × 3s + 10 measure × 3s; the single 1-fork outlierbench.06re-verified with 3 forks × 8 warmup + 15 measure):Geometric mean: -7.27%.
Changes
Evaluator.scala—visitExpris now the 7-type instanceof hot path (covers ~96.1% of dispatches) plus avisitExprColdtag-based tableswitch. Profiler instrumentation is preserved in the hot path —NewEvaluatorpreviously dropped it, so--profile --new-evaluatorwas silently broken.Settings.scala/Config.scala/SjsonnetMainBase.scala— drop the flag plumbing.Interpreter.createEvaluator— always returns the unifiedEvaluator.TestUtils/EvaluatorTests/AggressiveStaticOptimizationTests— drop theuseNewEvaluatorparameter;EvaluatorTestsno longer runs every assertion twice.bench/EvaluatorBenchmark.scala— deleted; ongoing perf coverage stays inMainBenchmarkandRegressionBenchmark.Net diff: 9 files, +270 / -530.
Breaking change
Settings.useNewEvaluatorand the--new-evaluatorCLI flag are removed. Embedders that explicitly set the flag should drop it; the resulting behavior is the same as the (now-default) hot-path dispatch.Test plan
./mill 'sjsonnet.jvm[3.3.7]'.compile./mill 'sjsonnet.js[3.3.7]'.compile./mill 'sjsonnet.native[3.3.7]'.compile./mill bench.compile./mill 'sjsonnet.jvm[3.3.7]'.test— 12 suites, 0 failures./mill __.checkFormat— clean