fix: sort ASCII-safe strings by runtime kind#868
Merged
Conversation
Motivation: AsciiSafeStr is a Val.Str subclass used by renderer optimizations, but std.sort compared exact runtime classes. Sorting arrays containing ASCII-safe strings could fail even though Jsonnet semantics treat them as strings. Modification: Classify sortable values by Jsonnet runtime kind instead of exact JVM/Native class, preserving existing fast paths for strings, numbers, arrays, booleans, and objects. Add a regression covering all-AsciiSafeStr, mixed Str/AsciiSafeStr, and key-function-produced AsciiSafeStr sorting. Result: std.sort now treats AsciiSafeStr values as strings without changing Jsonnet semantics. Full cross-platform validation will be run before opening the draft PR.
This was referenced May 23, 2026
stephenamar-db
pushed a commit
that referenced
this pull request
May 27, 2026
Motivation: Scala Native kube-prometheus rendering still showed write/output overhead after the renderer and strict JSON import stack. `NativeOutputStream` already bypasses the JVM-compatible `PrintStream` path by writing through C `fwrite`, but stdout still used the platform default stdio buffering. Key Design Decision: Keep this optimization Native-only and local to stdout buffering. Instead of changing renderer flush thresholds or `ByteBuilder` behavior globally, configure the C stdio stream with full buffering before any `NativeOutputStream` writes occur. Passing a null buffer lets libc own the buffer lifetime, so the Scala object does not need to retain native memory. Modification: - Configure `NativeOutputStream` with `setvbuf(file, null, _IOFBF, 256 KiB)` during construction. - Leave JVM, JS, YAML, expect-string, and file-output code paths unchanged. - Preserve existing explicit `flush()` behavior for trailing newline and close handling. Benchmark Results: Workload: `jrsonnet/tests/realworld/entry-kube-prometheus.jsonnet -J vendor` Candidate was benchmarked on the Scala Native 0.5.12 stacked exploration branch against clean `cf7b8af9`. | Order | Clean | Candidate | Result | | --- | ---: | ---: | ---: | | Forward mean | 218.848 ms | 188.528 ms | -13.9% | | Forward median | 215.517 ms | 187.368 ms | -13.1% | | Reverse mean | 224.045 ms | 183.701 ms | -18.0% | | Reverse median | 224.281 ms | 182.914 ms | -18.4% | Output equality matched by `cmp`. Validation: - `./mill --no-server --ticker false --color false __.reformat` - `./mill --no-server --ticker false --color false -j 1 __.test` — 444 passed, 0 failed - `./mill --no-server --ticker false --color false bench.runRegressions` Analysis: This is a lower-risk write/flush optimization than increasing `ByteBuilder` thresholds: it does not alter rendering order, JSON escaping, object materialization, or JVM/JS behavior. It only changes the buffering policy of the Native stdout `FILE*`, and explicit flushes still happen at the same public boundaries. References: - Scala Native 0.5.12 migration PR: #867 - Related performance stack context: #863, #864, #865, #866, #868 Result: Native stdout rendering writes fewer/smoother buffered chunks for large JSON output while preserving byte-identical output and the existing flush contract.
stephenamar-db
pushed a commit
that referenced
this pull request
May 27, 2026
Motivation: The Native stdout buffering follow-up showed that downstream buffering can materially reduce large-output write overhead. JSON `-o` output still sent `ByteRenderer` chunks directly to the file output stream, relying only on `ByteBuilder`'s internal flush threshold. Key Design Decision: Keep the change local to the JSON output-file fast path. Rather than changing `ByteBuilder` thresholds globally, wrap the file output stream in a `BufferedOutputStream` with the same 256 KiB output buffer size used for the Native stdout buffering follow-up. YAML, expect-string, stdout, and renderer semantics stay unchanged. Modification: - Add `OutputBufferSize = 256 * 1024` in `SjsonnetMainBase`. - Wrap JSON output-file `ByteRenderer` targets in `BufferedOutputStream(out, OutputBufferSize)`. - Flush the buffered stream at the same completion boundary before closing the underlying file output stream. Benchmark Results: Workload: `jrsonnet/tests/realworld/entry-kube-prometheus.jsonnet -J vendor -o /tmp/fileout-*.json` Candidate was benchmarked on the Scala Native 0.5.12 stacked exploration branch after the Native stdout buffering commit. | Order | Clean | Candidate | Result | | --- | ---: | ---: | ---: | | Forward mean | 217.372 ms | 205.062 ms | -5.7% | | Forward median | 196.625 ms | 183.491 ms | -6.7% | | Reverse mean | 210.517 ms | 177.174 ms | -15.8% | | Reverse median | 193.394 ms | 175.878 ms | -9.1% | Output equality matched by `cmp`. Validation: - `./mill --no-server --ticker false --color false __.reformat` - `./mill --no-server --ticker false --color false -j 1 __.test` — 444 passed, 0 failed - `./mill --no-server --ticker false --color false bench.runRegressions` Analysis: This preserves the existing rendering pipeline and only changes the buffering layer for file output. It avoids global `ByteBuilder` threshold changes, keeps stdout behavior separate, and does not affect YAML or expect-string paths. References: - Native stdout buffering PR: #869 - Scala Native 0.5.12 migration PR: #867 - Related performance stack context: #863, #864, #865, #866, #868 Result: Large JSON file output writes are buffered more effectively while preserving byte-identical output and the existing flush/close contract.
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
2 participants
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.
Motivation:
Val.AsciiSafeStris aVal.Strsubclass used by renderer/string optimizations.std.sortwas using exact runtime class equality to validate and dispatch sortable values, so arrays containingAsciiSafeStrcould fail even though Jsonnet semantics treat those values as strings.Key Design Decision:
Classify values by Jsonnet runtime kind rather than exact JVM/Native class. This preserves the existing specialized string/number/array sort paths while making subtype-based string optimizations semantically transparent to the standard library.
Modification:
SetModule.getClasschecks in default sorting and key-function sorting with sort-kind checks.new_test_suite/set_sort_ascii_safe_strings.jsonnetfor all-AsciiSafeStr, mixedStr/AsciiSafeStr, and key-function-producedAsciiSafeStrsorting.Benchmark Results:
This is a correctness/enabler PR, not a claimed performance win. On the Scala Native 0.5.12 stacked profiling branch, kube-prometheus output stayed byte-identical; sort-kind-only A/B was neutral: forward
194.3 ± 14.6 msclean vs187.9 ± 12.3 mscandidate, reversed170.7 ± 2.7 msclean vs172.0 ± 1.6 mscandidate.Analysis:
The exact-class check was brittle because optimized runtime value subclasses must remain semantically indistinguishable from their base Jsonnet type. The replacement also avoids closure-based
forall(_.getClass == keyType)checks in this path, but the important result is that futureAsciiSafeStrpropagation can be evaluated without breakingstd.sort.References:
AsciiSafeStrpropagation for the Scala Native 0.5.12 performance work.Result:
./mill --no-server --ticker false --color false __.reformatpassed../mill --no-server --ticker false --color false -j 1 __.testpassed 444/444.