Explain the restart() recursion where the refactor would happen - #160
Merged
Conversation
The KDoc already said not to route recovery through restart(), but the reason lives at the call site: restart() -> awaitWatchList() -> dismissExternalWindow(). That chain is invisible from the line it constrains, and the obvious cleanup is to have the recovery reuse restart(), which reintroduces it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e998b528-be91-4a05-a92e-97fb49e7ed79
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues; documentation-only change.
Pull request overview
This comment-only follow-up relocates the restart() recursion warning to the relevant recovery call site.
Changes:
- Clarifies the direct activity launch constraint.
- No runtime behavior changes.
File summaries
| File | Description |
|---|---|
baselineprofile/src/main/java/info/anodpslace/appwatcher/baselineprofile/AppWatcherJourney.kt |
Relocates and clarifies the recursion-prevention comment. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The two generated profiles have always been byte-identical -- git stored them as a single blob in every shipped build -- because one collector fed both. The split means regenerating now moves them by very different amounts, which reads like a broken generator to anyone cutting a release without that context. The PR body carrying this merged before the audience needed it, and a PR body is only read by reviewers anyway. The people who hit this are downstream of the merge, so the note belongs in the file they will be regenerating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e998b528-be91-4a05-a92e-97fb49e7ed79
| * | ||
| * The minimum required version of androidx.benchmark to generate a baseline profile is 1.2.0. | ||
| * | ||
| * Expect `baseline-prof.txt` and `startup-prof.txt` to differ. Until build 17107 a single collector |
The -285 was measured against master's committed profile at 30,011 lines, but regeneration may happen on a release branch, which is at 29,660. Against that anchor the same regenerated file is +66, not -285: the sign flips with the anchor, so the number misleads exactly where it was meant to reassure. git ls-tree showing two distinct blob ids is anchor-independent and exact. Also "until build 17107" excluded 17107, which is the last affected build -- 9bbb9657 still stores one blob under both paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e998b528-be91-4a05-a92e-97fb49e7ed79
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comment-only follow-up to #158, which merged two minutes before these landed (#158 at 17:49:32Z). Two notes, both moved to where the person who needs them will actually be looking.
1. The
restart()recursion, moved to the call site.#158 added a recovery path in
AppWatcherJourney.dismissExternalWindow()that relaunches the activity when the app has fallen back to the launcher, because back can never bring it forward from there. That path deliberately does not callrestart(), sincerestart() → awaitWatchList() → dismissExternalWindow()would recurse.That constraint was written in the function's KDoc header. Wrong place: the obvious cleanup is to have the recovery reuse
restart(), and whoever makes that change has their cursor on thescope.startActivityAndWait()line, eleven lines below a doc comment they are not editing.Reintroducing the recursion would not fail loudly. It would restore the ~145s-per-iteration timeout that took profile generation from 7m34s to 19m2s — the kind of regression nobody attributes to a harmless cleanup.
2. The profile divergence, moved into the generator.
#158 split one collector into two so the startup profile stops carrying journey classes. The consequence is that
baseline-prof.txtandstartup-prof.txtwill differ for the first time — they have been byte-identical in every shipped build, stored by git as a single blob, because one collector wrote both. Regenerating now moves startup by ≈-285 lines and baseline by ≈+7,400.Without context that reads as a broken generator, and the natural reaction is to "fix" it back. #158's body explained it, but that body merged before anyone downstream needed it, and a PR body is only ever read by reviewers. The audience here is whoever cuts a release months from now, so the note belongs in the file they will be regenerating.
No behaviour change in either commit. Rebased onto current
masterso it does not disturb #159 — the pre-rebase branch would have reverted it.