airbnb / airbnb/MagazineLayout

Proposal: Path to removing `_enableExperimentalOptimizations`

Open
#162 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Swift
Stars
3.4k
Forks
222
PR merge metrics
No merged PRs in 30d

Description

## Context

`MagazineLayout._enableExperimentalOptimizations` was added in #156 and documented as *"A temporary flag to enable safely testing some optimizations."* It's still default-`false` and now gates **35+ branches** across `MagazineLayout.swift`, `SectionModel.swift`, and `ModelState.swift` (PRs #158, #159, #160 each widened the surface).

I don't see a written exit plan, and wanted to put a proposal out there before more code lands behind the flag. Could be I'm missing context — happy to be told the flag is intentionally indefinite, in which case the rest of this doesn't apply.

## Why I think this matters

- Every PR behind the flag widens the divergence between the optimized and legacy paths.
- The optimized path currently has zero test coverage: `grep '_enableExperimentalOptimizations' Tests/` returns 0.
- Without parity tests + benchmarks, flipping the default is hard to do confidently.
- "Temporary" flags that live more than a couple of releases tend to become permanent forks.

## Proposed phases

**Phase 1 — Test parity** (started in #161).
Subclass the existing test classes and rerun every assertion with the flag flipped. Divergence between the optimized and legacy paths becomes a failed test. Started with `ModelStateLayoutTests`; the pattern extends mechanically to the other 4 model-state suites if you like the direction.

**Phase 2 — Benchmark harness.**
Add `MagazineLayoutBenchmarks` with XCTest `measure()` blocks for canonical scenarios:
- 1,000-item single section
- 100 sections × 10 items
- Deep self-sizing with preferred-height invalidations
- Repeated `invalidateLayout()` cycles

Capture baselines with flag=`false`, then quantify the wins with flag=`true`. Gives you numbers to back the flip and a CI gate against future regressions.

**Phase 3 — Flip the default to `true`.**
One-line change once Phases 1+2 are green. Public API surface doesn't change, but probably worth a minor version bump for visibility to consumers.

**Phase 4 — Remove the flag + delete the legacy paths.**
Cleanup PR. Same release as Phase 3 or the next minor — your call.

## Open questions

These I genuinely can't answer from outside:

1. **Is anyone using `_enableExperimentalOptimizations = true` externally?** The underscore prefix suggests SPI, but worth confirming you're not breaking a known consumer.
2. **Are there scenarios where the optimized path is intentionally divergent** from the legacy path? If yes, Phase 1 will surface them as test failures — would be good to mark them as expected up front.
3. **Release cadence** — does Airbnb's internal MagazineLayout consumer pin a version such that Phase 3 has to wait for a planned cut?
4. **Is anyone else thinking about this?** Don't want to step on an in-flight plan.

## What I'm offering

- Phase 1 is up at #161. Happy to extend the subclass pattern to the other model-state suites in a follow-up if the direction is right.
- Can take Phase 2 next — benchmark harness as a separate PR.
- Phases 3 and 4 are yours to land when you're comfortable.

If you'd rather just close this and #161 with a one-liner about why it's the wrong move, that's also a useful signal. Wanted to put the framing out there in case it's useful.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing _enableExperimentalOptimizations across MagazineLayout.swift, SectionModel.swift, and ModelState.swift, then inspect #161 and the existing ModelStateLayoutTests subclass pattern. Check the other four model-state suites and the proposed benchmark scenarios; done requires maintainer agreement on parity, benchmarks, external usage, and the staged removal plan.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
mobile-dev
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.