Skip to content

perf(densenet): O(n²) IndexOf-in-foreach in DenseBlock.SetExtraParameters - #1241

Merged
ooples merged 1 commit into
masterfrom
fix/post-1229-denseblock-indexof-on2
May 3, 2026
Merged

ooples merged 1 commit into
masterfrom
fix/post-1229-denseblock-indexof-on2

Conversation

@ooples

@ooples ooples commented May 3, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Replace _layers.IndexOf(layer) inside foreach with an index-tracked for loop in DenseBlock<T>.SetExtraParameters. The IndexOf only appears in the truncation error message but ran on every iteration, turning the loop O(n²).

Background

Reviewer-flagged in PR #1229 (PRRT_kwDOKSXUF85_NPlD). PR #1229 merged before the comment was addressed, so applying the fix here as a small follow-up.

Test plan

  • dotnet build clean on net10.0 + net471
  • DenseBlock-related tests still pass (no behavioral change — just moves IndexOf out of the hot path)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Improved internal iteration handling and error message formatting in layer serialization logic.

…enseBlock.SetExtraParameters

DenseBlock<T>.SetExtraParameters used `_layers.IndexOf(layer)` inside a
`foreach` loop. The IndexOf call only appears inside the truncation
error message, but it ran on every iteration and turned the loop
O(n²). Switched to an index-tracked `for` loop so the index is
available in O(1) for the error message and the happy path is plain
O(n).

Reviewer-flagged in PR #1229; PR was merged before the comment was
addressed, so applying the fix as a small follow-up here.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented May 3, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
aidotnet_website Ignored Ignored May 3, 2026 9:07pm
aidotnet-playground-api Ignored Ignored Preview May 3, 2026 9:07pm

Copilot AI review requested due to automatic review settings May 3, 2026 21:07
@coderabbitai

coderabbitai Bot commented May 3, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 35d3e631-f16f-4e35-8dac-7f67d3abd93f

📥 Commits

Reviewing files that changed from the base of the PR and between a8310eb and e8e9549.

📒 Files selected for processing (1)
  • src/NeuralNetworks/Layers/DenseBlock.cs

Walkthrough

The SetExtraParameters method in DenseBlock<T> replaces a foreach loop with an index-based for loop when iterating over sub-layers. The truncation error message now reports the current loop index instead of calling IndexOf(layer), eliminating a linear lookup from the error path while preserving all validation and child-layer serialization logic.

Changes

Layer Iteration & Error Reporting Optimization

Layer / File(s) Summary
Loop Structure & Error Path
src/NeuralNetworks/Layers/DenseBlock.cs
SetExtraParameters switches from foreach to index-based for loop over _layers. Truncation error message now uses loop index (DenseBlockLayer #{i}) instead of _layers.IndexOf(layer), removing the O(n) lookup from the error path. Offset tracking, child SetExtraParameters calls, and surplus-payload validation remain unchanged.

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

A loop once rode the foreach way,
With IndexOf slowing down the day,
Now an index marches, lean and fast,
Old lookups gone, errors unsurpassed. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the specific performance optimization in the changeset: replacing an O(n²) IndexOf call in a foreach loop with an index-tracked for loop in DenseBlock.SetExtraParameters.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/post-1229-denseblock-indexof-on2

Review rate limit: 3/5 reviews remaining, refill in 15 minutes and 38 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 applies a small follow-up performance fix in DenseNet’s DenseBlock<T> serialization path by removing an unnecessary List.IndexOf call from the SetExtraParameters loop. It keeps the same behavior while avoiding quadratic work in a method that iterates across the block’s child layers.

Changes:

  • Replace foreach + _layers.IndexOf(layer) with an index-based for loop in DenseBlock<T>.SetExtraParameters.
  • Preserve the truncation error message semantics while computing the layer index in O(1) per iteration.
  • Keep DenseBlock extra-parameter deserialization behavior otherwise unchanged.

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

@ooples
ooples merged commit 7785696 into master May 3, 2026
39 of 49 checks passed
@ooples
ooples deleted the fix/post-1229-denseblock-indexof-on2 branch May 3, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants