Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(critical-request-chains): prune requests without an initiator #11151

Merged
merged 4 commits into from
Jul 23, 2020
Merged

core(critical-request-chains): prune requests without an initiator #11151

merged 4 commits into from
Jul 23, 2020

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jul 22, 2020 •
edited
Loading

Fixes master CI unit tests

Summary
The slimmer third-party-web bundle in #11137 dropped the obscure entity we used for testing so I updated to one that won't disappear.

Hopefully we can reenable required checks again soon and get rid of CI that's always red to reserve the overall X signal 😅

UPDATE: after this PR was initially written, #11148 was autolanded before the required checks were turned back on which had critical request chain issues.

@patrickhulce patrickhulce requested a review from a team as a code owner July 22, 2020 21:38
@patrickhulce patrickhulce requested review from Beytoven and removed request for a team July 22, 2020 21:38
@paulirish
Copy link
Member

restarted these tests but this one is still failing

image

Beytoven
Beytoven approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@Beytoven Beytoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vercel vercel bot temporarily deployed to Preview July 22, 2020 23:39 Inactive
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Jul 23, 2020 •
edited
Loading

restarted these tests but this one is still failing

Yikes, so in between the time of fixing this one, @devtools-bot also auto-landed #11148 before unit_ubuntu-latest was required thanks to land-when-ci-is-green 🤦

This is actually a nice prompt to fix a long-standing bug in critical request chains we never ran into during our tests :)

Before, when a script is added to the head via script and doesn't have an initiatorRequest it automatically becomes "critical" because we only look at the priority in that audit and not any broader graph context. This led to weird dangling requests in CRC audit that weren't even part of the root document chain (see the identification of favicon.ico in sample json update as the "longest chain" which never made sense).

Now, we'll ignore any request in CRC that didn't have an initiator.

Long-term I think we should refactor critical request chains to be based on the page-dependency-graph instead of manually computing using a set of heuristics to approximate the graph.

This is a bigger change though, so the best course of action might be revert #11148, land this with just the 3p 1-liner fix and then reland #11148 with these CRC changes.

@patrickhulce
Copy link
Collaborator Author

Oof, actually my suggested course of action would require turning back off unit_ubuntu-latest as a required check since a revert of just #11148 won't pass CI 😬 Given how much trouble that got me into here, maybe we do just get this green again with the PR as-is?

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/computed/critical-request-chains-test.js
@@ -116,7 +116,7 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
}
);

it('returns correct data for two parallel chains', () => {
Copy link
Collaborator Author

@patrickhulce patrickhulce Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests were mostly 3-4 years old when this audit was very different, no real-world trace I found had a situation where this was desirable today

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/computed/critical-request-chains-test.js
@@ -168,11 +159,11 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
});
});

it('returns empty chain list when no critical request', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an impossible case

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/computed/critical-request-chains-test.js
@@ -182,26 +173,10 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
assert.deepEqual(criticalChains, {});
});

it('returns two single node chains for two independent requests', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same case as the above (pruning non-root document)

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/computed/critical-request-chains-test.js
@@ -222,27 +197,27 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
},
},
},
},
},
4: {
Copy link
Collaborator Author

@patrickhulce patrickhulce Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just whitespace indentation to nest them under root

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/results/sample_v2.json
},
"75994.33": {
"request": {
"url": "http://localhost:10200/favicon.ico",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

patrickhulce
patrickhulce commented Jul 23, 2020
View reviewed changes
lighthouse-core/test/results/sample_v2.json
"transferSize": 221
"duration": 4796.288000012282,
"length": 2,
"transferSize": 30174
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huge win IMO that favicon.ico at 221 is no longer flagged as the worst chain 🎉

@patrickhulce patrickhulce changed the title tests: update third-party-web test for entity with volume core(critical-request-chains): prune requests without an initiator Jul 23, 2020
@patrickhulce
Copy link
Collaborator Author

just talked to @paulirish in 1:1 and he's still 👍 on this and it's sat for about a day for folks to object soooo I'm gonna merge in interest of green. if there are strong feelings about the CRC audit then happy to bring up in a future refactoring PR :)

@patrickhulce patrickhulce merged commit 8b4803d into master Jul 23, 2020
@patrickhulce patrickhulce deleted the fix_3p_test branch July 23, 2020 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulirish paulirish paulirish approved these changes

@Beytoven Beytoven Beytoven approved these changes

Assignees

@Beytoven Beytoven

Labels
cla: yes waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@patrickhulce @paulirish @Beytoven @googlebot @devtools-bot