performance refactors by nmummau · Pull Request #870 · rr-wfm/MSBuild.Sdk.SqlProj

nmummau · 2026-03-21T00:46:57Z

My motivation for this pull request was to see if there was any low-hanging fruit changes that could be made to for performance gains.

each change explained

refactor: clarify analyzer override tests

I added tests around rule override behavior before changing code so the behavior contract is explicit and protected. The key gaps were in case-insensitive wildcard matching for both +! overrides and - suppressions. Those cases were previously untested in this repo path, so I added focused coverage.

perf(analyzer): reduce rule parsing overhead

I changed rule parsing to be single-pass and data-shape friendly:

BuildRuleLists now parses once instead of doing multiple filtered Split loops.
+! rules are normalized up front into:
- exact rule IDs (+!SRD0006), and
- wildcard prefixes (+!SRD* stored as SRD).
SqlRuleProblemExtensions.GetOutputMessage now checks those pre-normalized prefixes directly
while formatting, instead of slicing wildcard patterns repeatedly during output formatting.

perf(analyzer): speed up suppressor checks

I replaced per-problem linear suppression scans with an index:

Build a dictionary once per analysis run:
- Dictionary<RuleId, HashSet<SourceName>>
Normalize and store suppression source paths once when loading suppressions.
In SetProblemSuppressor, suppress-check is now:
- rule-id lookup → set membership check,
  instead of comparing each diagnostic against every suppression entry.

why this helps:

Before: each diagnostic did #suppressions comparisons (O(diag_count × suppress_count)).
After: each diagnostic does near-constant work (lookup + contains), plus one-time index build.

Concrete example:

2,000 diagnostics, 500 suppressions
- before: ~1,000,000 checks
- after: ~2,000 lookups + 500 index inserts

Validation:

Added/extended assertions around suppression handling (RunsAnalyzerWithSuppressionFile now
verifies suppressor processing is exercised).
Focused analyzer test sets pass on net8, net9, net10.

Net effect:

No behavior changes to outputs.
Safer scaling for large projects and suppression-heavy workloads.
Tests now lock in the exact behavior I optimized.

Copilot

Pull request overview

This PR targets build consistency and analyzer runtime performance by pinning C# language versions per target framework, optimizing rule/suppression handling, and adding/expanding tests to lock in override/suppression behavior.

Changes:

Build: set explicit LangVersion per TFM for multi-target projects (net8/net9/net10).
Perf/refactor: reduce analyzer rule parsing overhead and speed up suppression checks via precomputed structures.
Tests: add coverage for case-insensitive deploy property names and case-insensitive wildcard rule overrides/suppressions.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/DacpacTool.Tests/PropertyParserTest.cs	Adds test coverage for case-insensitive deploy property names.
test/DacpacTool.Tests/PackageDeployerTests.cs	Adds test coverage for case-insensitive deploy property names through `PackageDeployer`.
test/DacpacTool.Tests/PackageAnalyzerTests.cs	Adds coverage for case-insensitive wildcard override/suppression behavior and asserts suppressor processing is exercised.
test/DacpacTool.Tests/ExtensionsTest.cs	Adds coverage for reference external-parts precedence (literal vs variable).
test/DacpacTool.Tests/DacpacTool.Tests.csproj	Pins `LangVersion` per TFM for the test project.
src/DacpacTool/SqlRuleProblemExtensions.cs	Refactors wildcard severity override handling to use pre-parsed prefixes.
src/DacpacTool/PropertyParser.cs	Caches `DacDeployOptions` properties in a case-insensitive dictionary to avoid repeated reflection scans.
src/DacpacTool/PackageAnalyzer.cs	Single-pass rule parsing, pre-normalized wildcard prefixes, and indexed suppression checks (dictionary + hash sets).
src/DacpacTool/Extensions.cs	Caches regex instance; reduces reflection inside loops for SQLCMD variable metadata setting.
src/DacpacTool/DacpacTool.csproj	Pins `LangVersion` per TFM for the tool project.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nmummau · 2026-03-22T02:30:37Z

@ErikEJ Would you mind taking another look, and fire off a new Copilot review? Not sure how to do that.

nmummau · 2026-03-22T03:56:02Z

I'm removing the LangVersion change from this PR, and created a separate focused PR for that: #877

This here PR remains for performance refactors.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…e tests This improves parser/runtime internals that are invoked repeatedly and adds a focused test for multi-rule warning-to-error override parsing. Why - Avoid repeated allocation/lookup overhead in hot paths (reflection + regex reuse) - Keep behavior unchanged while making intent explicit and maintainable - Make analyzer override behavior easier to validate and reason about What changed - src/DacpacTool/PropertyParser.cs - Cache `DacDeployOptions` `PropertyInfo` objects in a dictionary keyed by name. - Replace per-call LINQ property scan with O(1) dictionary lookup. - This removes repeated reflection work when setting many deploy properties. - src/DacpacTool/Extensions.cs - Promote external parts parsing regex to a static readonly compiled instance. - Reuse it directly in `ParseExternalParts`. - Move `SetMetadata` method resolution outside the loop so we only fetch it once per call. - This reduces repeated regex construction and reflection churn. - src/DacpacTool/SqlRuleProblemExtensions.cs - Rewrite wildcard rule handling as an explicit loop over rules with a clear comment. - Preserve behavior while making wildcard `*` prefix matching easier to read and cheaper to maintain. - test/DacpacTool.Tests/PackageAnalyzerTests.cs - Create data-driven override test - Add dedicated test `RunsAnalyzer_WithMultipleWarningToErrorOverridesInSingleExpression` to verify multiple `+!` overrides in a single rule string all promote to error. Validation note - The new test runs under all target frameworks in `test/DacpacTool.Tests.csproj` (`net8.0`, `net9.0`, `net10.0`) because the test project is multi-targeted, so this is cross-target coverage.

- Add tests for case-insensitive wildcard rule/suppression behavior. - Parse rule expression once in PackageAnalyzer.BuildRuleLists, splitting +! entries into exact vs wildcard prefix buckets. - Pre-normalize wildcard overrides - Update SqlRuleProblemExtensions to evaluate wildcard prefixes from pre-normalized list. - Preserve existing behavior while lowering per-SqlRuleProblem work in hot diagnostic formatting path.

This refactor changes suppression matching in `PackageAnalyzer` from an O(problems × suppressions) scan to an indexed lookup. Before: - For each diagnostic (rule id + source file), the code checked every suppression entry in `StaticCodeAnalysis.SuppressMessages.xml`. After: - Build once per run: `Dictionary<RuleId, HashSet<SourceName>>` from suppression entries. - For each diagnostic: - lookup by `RuleId` - check `SourceName` in that set. What this means: - Example 2,000 diagnostics + 500 suppressions: - Old: ~1,000,000 comparisons - New: ~2,000 lookups + ~500 index builds Behavior: - No user-facing behavior change. - Same suppression decisions and logging semantics. Changes: - `src/DacpacTool/PackageAnalyzer.cs` - Added suppression index and replaced linear `Any(...)` scan with indexed check. - `test/DacpacTool.Tests/PackageAnalyzerTests.cs` - Added assertion in `RunsAnalyzerWithSuppressionFile` to ensure suppression processing is exercised (`"Suppressing rule:"`). Validation: - Focused analyzer tests pass on net8/net9/net10 for 9 tests.

Use the cached `SetMetadataMethod` in `AddSqlCmdVariables` instead of reflecting and null-checking the same method on every call. This keeps the method consistent with `AddReference`, removes duplicate reflection work, and preserves behavior.

Store wildcard warning-to-error prefixes in a case-insensitive `HashSet<string>` instead of a `List<string>` and update the consumer signature to match. This avoids redundant prefix scans when duplicate `+!...*` overrides are present.

since the cost of this optimization is not worth the gain, removing per PR review. Most invocations will have few parameters, and the cost of the optimization is not worth it.

provide a user-friendly error if a timeout occurs

PackageAnalyzer up to 94.8% coverage Extensions up to 85.6% coverage

jmezach · 2026-03-26T14:58:26Z

Hmm, I'm wondering what the coverage looks like now, but I'm not seeing that yet. Perhaps this needs to be rebased on master after merging #885?

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jmezach

LGTM

ErikEJ · 2026-03-31T14:05:24Z

@nmummau Could you look at the build error?

nmummau · 2026-03-31T16:46:36Z

@nmummau Could you look at the build error?

Fixed here #892. This was a result of the CI check we put in place to add dotnet format style to remove unused usings

ErikEJ reviewed Mar 21, 2026

View reviewed changes

Comment thread src/DacpacTool/DacpacTool.csproj Outdated

ErikEJ reviewed Mar 21, 2026

View reviewed changes

Comment thread test/DacpacTool.Tests/DacpacTool.Tests.csproj Outdated

ErikEJ reviewed Mar 21, 2026

View reviewed changes

Comment thread test/DacpacTool.Tests/PackageDeployerTests.cs

ErikEJ requested a review from Copilot March 21, 2026 06:54

Copilot started reviewing on behalf of ErikEJ March 21, 2026 06:54 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

Comment thread src/DacpacTool/PackageAnalyzer.cs

Comment thread src/DacpacTool/SqlRuleProblemExtensions.cs Outdated

Comment thread src/DacpacTool/PackageAnalyzer.cs Outdated

Comment thread src/DacpacTool/Extensions.cs Outdated

nmummau force-pushed the feat/explicit-langversion branch from 1943896 to 557cce3 Compare March 22, 2026 01:53

nmummau force-pushed the feat/explicit-langversion branch from 6edf6e2 to 09bbdee Compare March 22, 2026 03:55

nmummau changed the title ~~Explicit LangVersion per target, performance refactors~~ performance refactors Mar 22, 2026

ErikEJ requested a review from Copilot March 22, 2026 06:40

Copilot started reviewing on behalf of ErikEJ March 22, 2026 06:40 View session

Copilot AI reviewed Mar 22, 2026

View reviewed changes

Comment thread src/DacpacTool/PackageAnalyzer.cs Outdated

Comment thread src/DacpacTool/SqlRuleProblemExtensions.cs

jmezach reviewed Mar 23, 2026

View reviewed changes

Comment thread src/DacpacTool/Extensions.cs

jmezach reviewed Mar 23, 2026

View reviewed changes

Comment thread src/DacpacTool/PropertyParser.cs Outdated

nmummau force-pushed the feat/explicit-langversion branch 2 times, most recently from 39035d8 to e4e27d7 Compare March 23, 2026 23:19

nmummau added 10 commits March 25, 2026 21:07

fix: update comment for clarity

11357de

perf(analyzer): dedupe wildcard error prefixes

0022275

Store wildcard warning-to-error prefixes in a case-insensitive `HashSet<string>` instead of a `List<string>` and update the consumer signature to match. This avoids redundant prefix scans when duplicate `+!...*` overrides are present.

fix: remove micro-optimization

b8df69c

since the cost of this optimization is not worth the gain, removing per PR review. Most invocations will have few parameters, and the cost of the optimization is not worth it.

fix: handle external parts regex timeouts

bef8cbf

provide a user-friendly error if a timeout occurs

fix: make analyzer rule override matching case-insensitive

5e2228c

test: add tests to boost coverage

a4939fc

PackageAnalyzer up to 94.8% coverage Extensions up to 85.6% coverage

nmummau force-pushed the feat/explicit-langversion branch from 8dc8ac0 to a4939fc Compare March 26, 2026 01:07

nmummau requested a review from jmezach March 26, 2026 01:08

nmummau requested a review from ErikEJ March 26, 2026 01:08

ErikEJ requested a review from Copilot March 27, 2026 12:41

Copilot started reviewing on behalf of ErikEJ March 27, 2026 12:41 View session

ErikEJ requested a review from jeffrosenberg March 27, 2026 12:42

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Comment thread src/DacpacTool/Extensions.cs

jmezach approved these changes Mar 30, 2026

View reviewed changes

ErikEJ approved these changes Mar 31, 2026

View reviewed changes

ErikEJ merged commit 4fb639c into rr-wfm:master Mar 31, 2026
18 checks passed

Conversation

nmummau commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

each change explained

refactor: clarify analyzer override tests

perf(analyzer): reduce rule parsing overhead

perf(analyzer): speed up suppressor checks

why this helps:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nmummau commented Mar 22, 2026

Uh oh!

nmummau commented Mar 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmezach commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

jmezach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ErikEJ commented Mar 31, 2026

Uh oh!

nmummau commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nmummau commented Mar 21, 2026 •

edited

Loading