perf(apply): retire per-pixel guards Begin() already establishes, and record what the review measured

[maxderhak]

Summary

The tier-A/B half of the apply-path performance review, plus the results document that answers the question the review was set up to ask: did the stability and vulnerability hardening cost measurable throughput?

Almost none of it did. The guards the hardening commits added to per-pixel code are individually a compare against a member or a null test, and on this hardware they do not show up at all. The wins on this branch come from two things that are not guards — a gamma decode being redone on every call to the library's hottest function, and a zero-fill that was dead whenever a CLUT was present.

Measured

Two runs of best-of-8 interleaved A/B, re-verified after rebasing onto a master roughly forty commits newer:

case	run 1	run 2	re-verified	why
monochrome	+6.2%	+16.6%	+10.5%	single-entry gamma curve
mono-lab	+3.1%	+9.9%	+11.1%	same, plus a CLUT
matrix-trc	+3.6%	+4.4%	+3.0%	TRC curves
all others	within noise		within noise

The re-measurement matters because iccBenchApply's profile-root resolution was fixed in #2256 during the interval, which had made the benchmark vacuous on Windows — the platform all these numbers were taken on. It did not invalidate them: the same three cases move, in the same direction, by comparable amounts, and every checksum is identical to the master reference.

The wins land exactly on the curve-dominated cases, which is what the CIccTagCurve::Apply change predicts — those fixtures use curveType with a single u8Fixed8 entry, so every call took the gamma path that was re-decoding m_Curve[0] and computing an nIndex it then discarded.

What changed

• A6, CIccTagCurve::Apply — the hottest function in the library. Degenerate table sizes are tested before nIndex is computed rather than after, and the single-entry gamma is decoded once in Begin() instead of being rebuilt from m_Curve[0] on every call before going to pow().
• A4, three sampled-curve Applys: the isfinite(pos) guard is unreachable — v is already clamped into the sampled range and each class's Begin() refuses a zero span. The range clamps stay; they bound the index.
• A7, CIccToneMapFunc::Apply: Begin() already refuses unless the function type is 0 with enough params.
• A8, the two CAM elements: the channel-count check moves to CIccMpeCAM::Begin() rather than going away — Begin() previously refused only a null m_pCAM.
• A9, the two spectral emission matrices: Begin() returns false when the matrix could not be allocated and CIccTagMultiProcessElement::Begin() propagates that, so the null test and its zero-fill else branch were dead.
• A5, Interp2d's offset ceiling, computed in CIccCLUT::Begin() from values fixed by then instead of two multiplies and two subtractions per pixel.
• A3, CIccXformNDLut: the Begin() bound tightens from 256 to 16 and gains an output check, which lets both per-pixel min-with-16 clamps go. Also a correctness fix — Apply() copies into a fixed Pixel[16], so the old 256 bound admitted tags it could not apply and then silently truncated the transform.
• B2/B3/B4, the calculator: the debugger hook is read once instead of twice per opcode (it is a file-static, so the compiler had to reload it after every Exec), and two unreachable arithmetic guards are gone.

Every removal carries a comment saying whether its condition was decided by Begin() or was unreachable, so the argument can be re-derived rather than taken on trust.

What was deliberately not done, and why

• B1/B5. mpe-calc moved −1.4% — nothing, on the case wholly dominated by that loop. Attribution explains it: the first Mpe is 98% of the chain, that calculator has zero branch ops (so B1 has nothing to act on), and its sub-elements are CurveSets of FormulaSegments with exponents 1.0 and 2.4 — so pow() per channel per pixel. At 0.14 Mpx/s the cost is irreducible transcendental math, not interpreter overhead. That also explains why the previous branch's CLUT work moved mpe-calc 0.0%. The audit called tier B the highest-value target in the review. That was wrong, and wrong for a measurable reason: it counted checked-arithmetic call sites without asking what share of runtime they were.
• A1/A2, the per-pixel bounds in InterpND and Interp3dTetra. These need CIccCLUT::Begin() to be able to refuse, and it returned void. That prerequisite is supplied by fix: bound CLUT channel counts at every parser, and let Begin() refuse #2306, which also retires them — as a correctness change rather than a performance one, since tier C measured them at nothing and one of the two turned out to be a defect.

The results document

docs/superpowers/results/2026-08-20-tier-ab-results.md records the above, and also two things worth not repeating.

A reasoning failure. Two drafts of the A1/A2 section were wrong in opposite directions — the bounds were called "duplicated from Begin()", then "the only guard that works". Both were reasoning about reachability from the call chain rather than measuring it, and neither was true. Probing the library (during #2306) showed m_nInput > 16 never reached a CLUT through any read path: the element readers turn a failed Init() into a failed Read() via their GetData(0) NULL checks. The guards were unreachable, not load-bearing. The final commit here corrects that section with the measured evidence.

A process failure. Adding a member to CIccCLUT changes sizeof, and building only the benchmark target left sixty-odd regression binaries compiled against the old header but linked against the new DLL. That produced 16 failures — mostly SEGFAULTs plus one heap corruption — across unrelated tests, and a bisect that wrongly blamed A3. A full rebuild gave a clean suite on identical source. The failure mode invents failures and can equally hide real ones.

Verification

ctest --label-exclude known-red: 129/129, from a clean worktree on current master. All nine benchmark checksums identical to the master reference.

Merge order

Conflicts with #2306 in IccTagLut.{cpp,h}, IccCmm.cpp, IccMpeBasic.cpp and IccMpeSpectral.cpp. #2306 should land first — it makes CIccCLUT::Begin() return bool, which is the prerequisite this branch recorded as blocking A1/A2. Rebasing this branch afterwards resolves those hunks toward the version #2306 establishes, and lets the A1/A2 deferral be dropped rather than carried.

🤖 Generated with Claude Code

[xsscx]

2026-08-25 18:52:44 UTC

[Copilot]

Pull request overview

This PR optimizes iccDEV’s apply-path hot loops by hoisting invariants into Begin() and removing per-pixel guards that are asserted to be unreachable once setup (Begin()) has succeeded. It also adds a results write-up documenting the measured throughput impact of the hardening-era checks and the performance changes made here.

Changes:

• Hoist per-call/per-pixel recomputation into Begin() (e.g., curve gamma decode, CLUT 2D offset ceiling) and remove redundant guards in several Apply() paths.
• Tighten CIccXformNDLut::Begin() channel-count validation to match fixed scratch storage, removing per-pixel clamps and turning silent truncation into a setup-time failure.
• Add a tier A/B performance results document; bump OpenSSL-related pinned package versions in the container.

Reviewed changes

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

Show a summary per file

File	Description
IccProfLib/IccTagLut.h	Adds CIccTagCurve::Begin() declaration and caches single-entry gamma; adds CIccCLUT cached ceiling for 2D interpolation.
IccProfLib/IccTagLut.cpp	Implements CIccTagCurve::Begin() gamma precompute; computes cached CLUT 2D ceiling during CIccCLUT::Begin().
IccProfLib/IccMpeSpectral.cpp	Removes null guards in emission matrix Apply() based on Begin()-established invariants.
IccProfLib/IccMpeCalc.cpp	Reduces calculator interpreter overhead by caching debugger hook and removing unreachable checked arithmetic in the opcode loop.
IccProfLib/IccMpeBasic.cpp	Removes isfinite(pos) guards from sampled-curve applies based on assumptions about clamping and Begin() preconditions.
IccProfLib/IccCmm.h	Adds CIccXform3DLut flag to conditionally skip dead scratch zero-fill when CLUT is present.
IccProfLib/IccCmm.cpp	Gates scratch zero-fill in CIccXform3DLut::Apply(); tightens CIccXformNDLut::Begin() bounds to 16 and removes per-pixel clamps.
docs/superpowers/results/2026-08-20-tier-ab-results.md	New document recording tier A/B measurements and methodology/process notes.
Dockerfile	Bumps pinned libssl-dev and openssl-provider-legacy package versions.

Suppressed comments (3)

IccProfLib/IccMpeBasic.cpp:1229

• pos can still become NaN/Inf on malformed tags even if v is clamped, because m_range (derived from untrusted profile endpoints) is not validated as finite in Begin() (it only checks m_endPoint-m_startPoint == 0.0). With the !std::isfinite(pos) guard removed, pos may be non-finite and then gets cast to icUInt32Number, which is undefined behavior and can lead to out-of-bounds reads from m_pSamples.

This issue also appears in the following locations of the same file:

• line 1849
• line 2495

  // No isfinite guard: v is clamped into [m_startPoint, m_endPoint] above, and
  // Begin() refuses a zero span (m_endPoint-m_startPoint == 0.0 returns false),
  // so pos cannot be non-finite. The clamps stay -- they bound the index.
  icFloatNumber pos = (v-m_startPoint)/m_range * m_last;
  if (pos<0.0f)
    pos=0.0f;
  else if (pos>m_last)
    pos=m_last;

  icUInt32Number index = static_cast<icUInt32Number>(pos);
  icFloatNumber remainder = pos - (icFloatNumber)index;

IccProfLib/IccMpeBasic.cpp:1858

• pos is cast to icUInt32Number a few lines below; without the removed !std::isfinite(pos) guard, malformed curve parameters (e.g., non-finite m_firstEntry/m_lastEntry or NaN m_range) can produce NaN/Inf pos and trigger undefined behavior on the cast and subsequent sample indexing.

  // No isfinite guard: v is inside [m_firstEntry, m_lastEntry] on this path, and
  // Begin() refuses m_range == 0 and m_last == 0, so pos cannot be non-finite.
  icFloatNumber pos = (v-m_firstEntry)/m_range * m_last;
  if (pos<0.0f)
    pos=0.0f;
  else if (pos>m_last)
    pos=m_last;

  icUInt32Number index = static_cast<icUInt32Number>(pos);
  icFloatNumber remainder = pos - (icFloatNumber)index;

IccProfLib/IccMpeBasic.cpp:2504

• Even though v is forced finite, pos can still be non-finite if m_range is NaN/Inf due to malformed endpoints (untrusted input). Removing the !std::isfinite(pos) guard means pos may be NaN/Inf and then gets cast to icUInt32Number, which is undefined behavior and can lead to out-of-bounds reads from m_pSamples.

  // No isfinite guard: same argument as CIccSingleSampledCurve::Apply -- v is
  // inside the sampled range here and Begin() refuses a zero range.
  icFloatNumber pos = (v - m_firstEntry) / m_range * m_last;
  if (pos<0.0f)
    pos=0.0f;
  else if (pos>m_last)
    pos=m_last;

  icUInt32Number index = static_cast<icUInt32Number>(pos);
  icFloatNumber remainder = pos - (icFloatNumber)index;