Minor refactoring of nsRefreshDriver::Tick
Categories
(Core :: Layout, task)
Tracking
()
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(9 files)
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-esr115+
|
Details | Review |
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
|
Details | Review |
Filing this bug to do some refactoring in nsRefreshDriver::Tick, to make that massive function slightly-less-massive and to make debugging a bit more straightforward.
Assignee | ||
Comment 1•7 months ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Assignee | ||
Comment 2•7 months ago
|
||
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Depends on D191908
Assignee | ||
Comment 3•7 months ago
|
||
Try (static-analysis to answer the annotation question in part 1, and linux-reftests to get some minimal functional testing, even though this shouldn't change behavior):
https://treeherder.mozilla.org/jobs?repo=try&revision=d04e24ce6e3d25131babb2819c90e194ac71ac1e
Assignee | ||
Comment 4•7 months ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Depends on D191909
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7eddf33b21eb part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/e92559c7af9c part 2: Unroll the observer-array-loop in nsRefreshDriver::Tick. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/f19db7c059b3 part 3: Coalesce redundant array-index checks in nsRefreshDriver. r=tnikkel
Comment 6•7 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7eddf33b21eb
https://hg.mozilla.org/mozilla-central/rev/e92559c7af9c
https://hg.mozilla.org/mozilla-central/rev/f19db7c059b3
Assignee | ||
Comment 7•7 months ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Original Revision: https://phabricator.services.mozilla.com/D191908
Updated•7 months ago
|
Assignee | ||
Comment 8•7 months ago
|
||
This patch doesn't change behavior.
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Original Revision: https://phabricator.services.mozilla.com/D191909
Updated•7 months ago
|
Assignee | ||
Comment 9•7 months ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Original Revision: https://phabricator.services.mozilla.com/D191911
Updated•7 months ago
|
Comment 11•7 months ago
|
||
Uplift Approval Request
- Risk associated with taking this patch: Low
- Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
- Needs manual QE test: no
- Fix verified in Nightly: yes
- Steps to reproduce for manual QE testing: N/A
- User impact if declined: None
- Code covered by automated testing: yes
- String changes made/needed: None
- Is Android affected?: no
Comment 12•7 months ago
|
||
We should take this on Beta as well if it's wanted on ESR this cycle.
Assignee | ||
Comment 13•7 months ago
|
||
Sounds good. For whatever reasons, the crashes that this might help with are most-prevalent on ESR so that's where I ultimately care about this reaching; but there's no problem having it on beta as well. I'll request uplift there.
Assignee | ||
Comment 14•7 months ago
|
||
This doesn't affect behavior; it's just a pure refactoring, moving code from
being inline to being in a helper function.
Original Revision: https://phabricator.services.mozilla.com/D191908
Updated•7 months ago
|
Assignee | ||
Comment 15•7 months ago
|
||
This patch doesn't change behavior.
This is a bit gross, but it helps us make better use of the backtrace to figure
out what's going on in a given crash report.
Original Revision: https://phabricator.services.mozilla.com/D191909
Updated•7 months ago
|
Assignee | ||
Comment 16•7 months ago
|
||
This patch doesn't change behavior; it just removes some redundant checks.
(Note that the tight-scoping around nsAutoMicroTask/ReduceAnimations is
important for the RAII object to be torn down at the intended time; but it
doesn't need its own if-check.)
Original Revision: https://phabricator.services.mozilla.com/D191911
Updated•7 months ago
|
Comment 17•7 months ago
|
||
Uplift Approval Request
- Is Android affected?: no
- String changes made/needed: None
- Code covered by automated testing: yes
- User impact if declined: None
- Steps to reproduce for manual QE testing: N/A
- Fix verified in Nightly: yes
- Needs manual QE test: no
- Explanation of risk level: No behavior change; this patch-stack is purely a refatoring of existing code.
- Risk associated with taking this patch: Low
Comment 18•7 months ago
|
||
Comment on attachment 9360456 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r?tnikkel
Approved for 120.0b4
Updated•7 months ago
|
Updated•7 months ago
|
Updated•7 months ago
|
Comment 19•7 months ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f28354147bcf https://hg.mozilla.org/releases/mozilla-beta/rev/e67b162f7903 https://hg.mozilla.org/releases/mozilla-beta/rev/f7a6096fda1c
Comment 20•7 months ago
|
||
Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
Approved for 115.5esr
Updated•7 months ago
|
Updated•7 months ago
|
Comment 21•7 months ago
|
||
Comment on attachment 9360450 [details]
Bug 1861259 part 1: Refactor out a helper function for loop body in nsRefreshDriver::Tick. r=tnikkel
:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820
Updated•7 months ago
|
Assignee | ||
Comment 22•7 months ago
•
|
||
(In reply to Dianna Smith [:diannaS] from comment #21)
:dholbert the esr115 patches need to be rebased with esr115 due to conflicts with bug 1830820
Hmm, it's not actually a conflict with that bug; it's rather just that the contextual code is different on ESR115. (The uplift for bug 1830820 actulaly removed a difference from esr115 vs. central, and hence made for a cleaner rebase operation here, not a messier one. But there are still other contextual differences that lando didn't detect when generating the ESR-specific uplift revisions.)
(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)
Assignee | ||
Comment 23•7 months ago
|
||
Specifically: esr115-rebase-wise, this snippet has a EvaluateMediaQueriesAndReportChanges()
call on mozilla-central (and beta) but not on ESR115, because it was added in bug 1451717:
https://searchfox.org/mozilla-central/rev/4f15c940fa206cb29e4c6992a9d8ba2eb4e7df88/layout/base/nsRefreshDriver.cpp#2619-2621
Here's the equivalent snippet on esr115, missing that middle line (because bug 1451717 didn't land there):
https://searchfox.org/mozilla-esr115/rev/d39c91545f0f5086bb26e2dee46d31ae5344b2b0/layout/base/nsRefreshDriver.cpp#2616-2617
This is ~fine, but it does cause a merge conflict -- the "part 1" patch here is moving that whole chunk of code from one place to another, so that trivially creates a merge conflict. (Trivial for a human to work around, but should have been detected/flagged when generating the lando ESR uplift request, presumably.)
Assignee | ||
Comment 24•7 months ago
•
|
||
The other merge-conflict is from some telemetry code that we removed on central in https://hg.mozilla.org/mozilla-central/rev/65677e6aa05d#l1.57 but still exists on ESR115.
That change is slightly awkward to rebase around (since it involves some state that straddles code that's now split between two functions).
Given that the lines in question are just for unused telemetry (per the commit message in the commit that removed it from central), it seems like we should just uplift that patch as well, rather than trying to delicately edit this bug's patch to preserve those lines to still collect apparently-unused telemetry.
Assignee | ||
Comment 25•7 months ago
•
|
||
So I'd like to request ESR115 uplift for bug 1843534's https://phabricator.services.mozilla.com/D185007 specifically [EDIT: here's the esr115 uplift version of that patch] as a prereq to the patch stack here. (It's the patch that's creating a merge conflict per comment 24). It's a pure unused-code-removal (with the removed code being otherwise slightly awkward to work around).
Not sure what the cleanest way to do that is. I'll start out by trying to do a specific uplift request for that patch via moz-phab if I can figure out how to do that without creating uplift-requests for the rest of the patches on that bug...
Assignee | ||
Comment 26•7 months ago
|
||
ESR115 patches (part 1 in particular) is now updated to:
(1) work around the missing line in the code-that's-being-moved-with-no-changes (per comment 23)
(2) be based off of the code-removal referenced in comment 24 - comment 25 (which makes parts 1 and 2 here simpler to rebase & reduces opportunity for human error). I've incorporated that into the ESR115 patch stack via "edit related revisions".
I've verified that the patch stack here now applies cleanly onto up-to-date mozilla-esr115 (with moz-phab patch D191987 --apply-to esr115
).
DiannaS: sorry for the trouble; the ball's back in your court, and this is good to land assuming you're OK to approve the uplift request on the supporting code-removal patch.
Assignee | ||
Comment 27•7 months ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #22)
(I don't expect lando/phabricator to be able to resolve those conflicts, but it's odd that it didn't notice them.)
side note: I filed bug 1861780 on making the lando-based uplift process more helpful here, FWIW.
Comment 28•7 months ago
•
|
||
Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.
nvm i think i fixed it will land shortly
Updated•7 months ago
|
Assignee | ||
Comment 29•7 months ago
|
||
Thanks!
Comment 30•7 months ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr115/rev/3e67c049c9d5 https://hg.mozilla.org/releases/mozilla-esr115/rev/9044fbb8cfd0 https://hg.mozilla.org/releases/mozilla-esr115/rev/b3b57a7219e9
Updated•7 months ago
|
Updated•7 months ago
|
Comment 31•7 months ago
|
||
(In reply to Dianna Smith [:diannaS] from comment #28)
Thank you!!!
The only issue i have now is that https://phabricator.services.mozilla.com/D191985 seems to be for m-c instead of esr115. not sure what happened otherwise i could land them all at once base on the dependencies you set. Im gonna take bug 1843534 for now.nvm i think i fixed it will land shortly
The repo was probably set to mozilla-central when the patch was updated due to bug 1828731
Assignee | ||
Comment 32•7 months ago
|
||
Yup, that sounds likely - thanks for the reference. (I did use moz-phab submit
when updating the uplift patch here.)
Description
•