Skip to content

Remove redundant range checks in Uri scheme parsing#124284

Merged
MihaZupan merged 1 commit intomainfrom
MihaZupan-patch-2
Feb 12, 2026
Merged

Remove redundant range checks in Uri scheme parsing#124284
MihaZupan merged 1 commit intomainfrom
MihaZupan-patch-2

Conversation

@MihaZupan
Copy link
Member

Now that #121262 is fixed, these are no longer needed (and are in fact just overhead)

@MihaZupan MihaZupan added this to the 11.0.0 milestone Feb 11, 2026
@MihaZupan MihaZupan requested a review from EgorBo February 11, 2026 16:33
@MihaZupan MihaZupan self-assigned this Feb 11, 2026
Copilot AI review requested due to automatic review settings February 11, 2026 16:33
@MihaZupan
Copy link
Member Author

@MihuBot

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes redundant bounds checks in Uri scheme parsing now that the JIT can elide dependent range checks correctly (per #121262), reducing unnecessary overhead in a hot parsing path.

Changes:

  • Simplified the early validation condition in ParseSchemeCheckImplicitFile by removing now-redundant (uint)i / (uint)(i + 1) range checks.

@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #124284

Holistic Assessment

Motivation: ✅ The PR is justified. PR #124242 (merged today) improved the JIT to recognize that (uint)(i + cns) < (uint)length implies (uint)(i + smallerCns) < (uint)length. The redundant checks being removed here were explicit hints for bounds check elimination that are no longer needed.

Approach: ✅ Correct approach. The remaining condition (uint)(i + 2) >= (uint)uriString.Length || colonOffset == 0 properly guards all subsequent accesses to uriString[i], uriString[i+1], and uriString[i+2] in lines 3567-3596.

Summary: ✅ LGTM. Simple, correct cleanup that removes dead code following a JIT optimization. The change is a net positive — it removes unnecessary overhead and improves code clarity.


Detailed Findings

✅ Correctness — Bounds checks remain valid

Verified that after the simplified guard at line 3555-3556, all subsequent accesses are safe:

  • uriString[i] — accessed at lines 3570, 3586
  • uriString[i + 1] — accessed at lines 3567, 3583, 3589
  • uriString[i + 2] — accessed at line 3572

The check (uint)(i + 2) >= (uint)uriString.Length returning early ensures i, i+1, and i+2 are all valid indices when execution continues past the guard.

✅ JIT Fix Verification — Dependency merged

Confirmed that PR #124242 ("Remove bounds checks for 'i + cns < len' pattern") was merged at 2026-02-11T16:18:03Z, which is the prerequisite for this cleanup.

💡 Minor — Comment could be removed

The comment // NB: A string must have at least 3 characters and at least 1 before ':' at line 3554 is still accurate, but the now-deleted comment // Redundant checks to eliminate range checks below is the one that explained why those checks existed. No action needed since the deleted lines and their comment are being removed together.

@MihaZupan MihaZupan enabled auto-merge (squash) February 12, 2026 00:38
@MihaZupan MihaZupan merged commit 1befada into main Feb 12, 2026
93 of 100 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants