Skip handler dispatch for setters under MaxLogLevelNone#465
Merged
Conversation
Motivation * The root `Package.swift` and other targets are on `swift-tools-version:6.2`. The three benchmark manifests were left at `6.1` by oversight. Modifications * Update `swift-tools-version` from `6.1` to `6.2` in `Benchmarks/Package.swift`, `Benchmarks/NoTraits/Package.swift`, and `Benchmarks/MaxLogLevelWarning/Package.swift`. Result * All benchmark manifests target the same Swift tools version as the rest of the repository.
Motivation
* When `MaxLogLevelNone` is enabled every `Logger.trace`/`debug`/.../
`critical` method and `Logger.log(level:)` compiles down to an empty
body, so no log statement can ever observe what the underlying
`LogHandler` sees.
* Despite this, the `subscript(metadataKey:)` and `logLevel` setters
still went through the handler. Code like
`logger[metadataKey: "user-id"] = "\(id)"` would CoW the logger
storage and call into the handler even though nothing could read the
metadata back out via a log statement.
Modifications
* Wrap the bodies of `Logger.subscript(metadataKey:) { set }` and
`Logger.logLevel { set }` in `#if !MaxLogLevelNone`, mirroring the
existing guards on the level-specific log methods.
* Note this behaviour in `DisableLogLevelsDuringCompilation.md` so
callers know reads still go through the handler but writes do not.
Result
* `logger[metadataKey:] = ...` and `logger.logLevel = ...` compile to
empty bodies when `MaxLogLevelNone` is set, matching the
zero-runtime-overhead guarantee the trait already provides for log
emission.
Motivation * `Benchmarks/MaxLogLevelWarning/` already pins `objectAllocCount: 0` for its trait via `package-benchmark` thresholds, so any regression that re-introduces a path through the `LogHandler` would fail CI. * There was no equivalent subpackage for `MaxLogLevelNone`, which is what let the metadata subscript and `logLevel` setters slip through unguarded. Modifications * Add `Benchmarks/MaxLogLevelNone/`, mirroring `Benchmarks/MaxLogLevelWarning/`: a `Package.swift` depending on `swift-log` with the `MaxLogLevelNone` trait, a `.gitignore`, and benchmark cases for `critical`, `log(level: .critical, ...)`, `logger[metadataKey:] = ...`, and `logger.logLevel = ...`. * Check in the `Xcode swift 6.3` threshold baselines so each benchmark asserts `objectAllocCount: 0` and a p90 instruction ceiling. * Add a `macos-benchmarks-MaxLogLevelNoneBenchmarks-trait` job to both `pull_request.yml` and `main.yml`, requesting Swift 6.3 to match the other trait benchmark job, chained after the `MaxLogLevelWarning` job to keep the macOS benchmark runs serialised. Result * A future change that re-introduces handler dispatch for any of the guarded paths under `MaxLogLevelNone` trips the `objectAllocCount: 0` threshold and fails CI.
rnro
force-pushed
the
reduce_MaxLogLevelNone_costs
branch
2 times, most recently
from
54bc472 to
573c380
Compare
Motivation * The relative p90 instruction threshold in `BenchmarksFactory` is set to 2%, but on the macOS CI runner the same source produces results that swing by up to ~8% between back-to-back runs (worst observed was the `MaxLogLevelNone` `_metadata_set` benchmark at +7.6%), so the threshold trips on noise rather than on real regressions. * Chasing the moving baseline by repeatedly amending the checked-in threshold JSONs doesn't fix this — each refresh would just need to be refreshed again. Modifications * Bump the `.instructions` relative p90 threshold from `2.0` to `10.0` in `Benchmarks/Sources/BenchmarksFactory/MakeBenchmark.swift`. * Leave the `.objectAllocCount` absolute threshold of `0` alone — that is the load-bearing invariant the benchmarks exist to police. Result * Normal runner-to-runner instruction count jitter no longer fails the benchmark jobs while still flagging anything that drifts more than 10%.
rnro
force-pushed
the
reduce_MaxLogLevelNone_costs
branch
from
573c380 to
b29c6a0
Compare
kukushechkin
approved these changes
May 21, 2026
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.
Skip handler dispatch for setters under
MaxLogLevelNoneMotivation
MaxLogLevelNoneis enabled everyLogger.trace/debug/.../criticalmethod andLogger.log(level:)compiles down to an empty body, so no log statement can ever observe what the underlyingLogHandlersees.subscript(metadataKey:)andlogLevelsetters still went through the handler. Code likelogger[metadataKey: "user-id"] = "\(id)"would CoW the logger storage and call into the handler even though nothing could read the metadata back out via a log statement.Modifications
Logger.subscript(metadataKey:) { set }andLogger.logLevel { set }in#if !MaxLogLevelNone, mirroring the existing guards on the level-specific log methods.DisableLogLevelsDuringCompilation.mdso callers know reads still go through the handler but writes do not.Result
logger[metadataKey:] = ...andlogger.logLevel = ...compile to empty bodies whenMaxLogLevelNoneis set, matching the zero-runtime-overhead guarantee the trait already provides for log emission.