core(fr): add computed cache to pass context

[adamraine]

Gatherers that use networkRecords in loadData will not have access to networkRecords in getArtifact. Our options to resolve this are:

1. Recomputed the network records in each gatherer.
2. Make computedCache available in gatherers and request the NetworkRecords like a normal computed artifact.
3. Make NetworkRecords into a FR gatherer and reference it as a dependency.

I haven't experimented much with option 3, not sure if it's even possible. This PR does option 2, using LinkElements as an example for what this will look like.

[patrickhulce]

I love the removal of computed artifact dependency on auditing! Anything that brings our misnomer computed artifacts closer to their true memoized functions existence, SGTM :)

Also heads up, I would expect you to need to update the various FR runners as well to use the computed cache.

[patrickhulce]

FRTransitionalContext was supposed to be the minimal set of shared context ("The limited context interface shared between pre and post Fraggle Rock Lighthouse.").

I get that dependencies somewhat violated that by always being {}, and I'm guessing that's what prompted this addition? I like the explicitness. Perhaps we should enforce that at the shared FRGatherer implementation level? Like if the meta contains dependencies throw an error in afterPass that it must be overriden to handle dependencies?

[adamraine]

How do you feel about these changes adding dependencies: {} to PassContext?

[patrickhulce]

I'm not sure that solves the issue I have in mind, but maybe it's aligned and just doesn't affect things, so I'm open to it.

I'm thinking all afterPass methods should be typed PassContext and all getArtifact should be typed FRTransitionalContext. In base-gatherer's afterPass method at runtime we check if there are any dependencies defined in the meta, if there are we throw an error immediately and don't invoke the other methods.

From that lens it doesn't really matter if dependencies is defined on PassContext because nothing is going to attempt to access dependencies on something that is typed PassContext anyway.

[adamraine]

Ah I think I see now.

[patrickhulce]

In the first attempt at sharing, I believe a primary objection was the references to these other pieces of state that computed artifacts have.

There's no reason computed artifacts actually need settings or options it's just an artifact (heh) of the fact that audits always have access to this information. If we remove both of those from the typedefs (which we should to make it more generic and lower the burden of additional context here), context can be used directly without this intermediate object.

We only have two computed artifacts to fix to remove this requirement.

lighthouse-core/computed/metrics/timing-summary.js:34:74
lighthouse-core/computed/resource-summary.js:56:53 (which was just a minor mistake in the first place)

[adamraine]

Actually it's more than two. Timing summary needs settings to request a bunch of lantern metrics, which use settings.throttlingMethod. So we need to fix LanternMetric as well.

Nevermind, I see they don't use the settings from context.

[brendankenny]

lighthouse-core/computed/metrics/timing-summary.js:34:74

this one's a pretty serious bug that luckily we don't end up triggering. If someone requested timing-summary with the same trace but once with mobileSlow4G and once with desktopDense4G, they'd get the same answers back because caching is only on the trace and devtoolsLog :/

The intention was definitely to only pass along the computedCache, but the type should have been more prescriptive that that was the case rather than just optimizing for ease of use.

[brendankenny]

3. Make NetworkRecords into a FR gatherer and reference it as a dependency.

I haven't experimented much with option 3, not sure if it's even possible.

Yeah, it's not. NetworkRecords contains cycles, so isn't serializable via JSON.

[brendankenny]

This seems like a natural extension of and need for artifact dependencies, so generally LGTM.

[brendankenny]

One thing I'd like to see in general, though, is less use of computed artifacts :)

• When we really need a cached result it means the computation was slow. That's what computed artifacts are there for, but OTOH, slow computations are things that shouldn't be happening in the gathering stage.
• When the computation isn't slow, computed artifacts are an over-complicated way to simply call a function. In a longish run of the verge I just took, for instance, the median lh:computed:* timing is 0.97ms (albeit a decent number of them are single-file UnusedJavascriptSummary calls). That's a lot of ceremony for not much savings in a lot of cases. We also don't record how much time we save by memoized calls, nor do we record how slow ArbitraryEqualityMap is if e.g. we've cloned or spread an object and it has to do some level of deep equality comparisons instead of simple strict equality :/

In any case, we should move away from defaulting shared functions to computed artifacts unless measured performance during development warrants it, and if a function is slow enough to warrant a computed artifact, we should still be working to speed it up because--even with caching--we're still paying for that computation somewhere the first time it's run.

[adamraine]

So this PR is doing three things now:

1. Remove the settings and options from the context for computed artifacts.
2. Add computedCache to the context for FR gatherers.
3. Convert LinkElements to FR.

I think I am going to move item 1 to a separate PR, and focus on the remaining items in this PR.

[adamraine]

Should we be sharing computed artifacts between phases?

[patrickhulce]

Yes, but I don't imagine it will really matter in practice as there are no dependencies for anything but getArtifact

[patrickhulce]

sweet!

[brendankenny]

LGTM!