core: initial refactor of computedArtifact import/caching #5907

brendankenny · 2018-08-24T22:54:29Z

Proof of concept of moving computed artifacts off of the artifacts object passed into audits and just use regular require('./path/to/computed-artifact.js') to use them.

Starts with a really simple computed artifact, manifest-values.js, which doesn't need to request any computed artifacts itself, nor does any computed artifact request it, but it is non-trivial and required in four audits. The computed artifact cache is now part of LH.Audit.Context, and audits need to pass in the cache rather than it being created and bound behind the scenes.

Nice thing about this approach is non-converted computed artifacts can continue to function as-is and we can convert them one by one (or in bunches) rather than in one massive, unreadable PR.

There are a few things we lose without the automatically created methods, but I think we'll have a huge step up in being able to analyze dependencies, trace logic through, etc. The fact that this was so difficult to untangle just for a super-simple computedArtifact shows the downsides of the earlier magic experience. Using computed artifacts (or moving off them) in an audit is now as easy as pulling in any other library module.

This was part of #5008 under "Nice to have". I swear we had a discussion about this in a set of comments somewhere, but the earliest reference I could find was #4961 (review), so maybe it was in person.

brendankenny · 2018-08-24T22:57:13Z

whoops, wanted it to be the cache passed in, not the whole context. Just sec :)

brendankenny · 2018-08-24T23:11:22Z

Done.

Only ergonomic downside is that, since these are essentially single function calls with no state (that isn't cacheable and so needs to be passed in), they really cry out to be static. However, typescript doesn't support polymorphic this on static methods, so we can't have both automatic caching and type checking (and completion) based on whatever the computed artifact class needs as input/gives as output.

We really want type checking on those, but I think we also want automatic caching. Repeating the boilerplate everywhere would be dumb, and it's really easy to get slightly wrong and not notice it's recomputing some expensive calculation (a bug that did this was the original impetus for automatic caching - #675).

So, as a result, you have to do const result = new ComputedArtifact().request(cache, input);. Not ideal, but not that bad, and if typescript ever fixes microsoft/TypeScript#5863 and these are still around, some future Lighthouse person can do a quick fix for them all :)

patrickhulce · 2018-08-25T00:05:07Z

@brendankenny WDYT about something like this instead?

brendankenny · 2018-08-25T01:06:46Z

WDYT about something like this instead?

well I spent the last day learning everything about the limits of polymorphic this in typescript to make my way work, but I think I like this one better :)

I'll try it out.

brendankenny · 2018-09-11T03:14:09Z

@paulirish how do you feel about this format

patrickhulce

I like this a lot. My only gripe right now is the churn to all of our context passing.

Just thinking out loud here, what are the primary benefits of attaching the cache to context? If each computed artifact just kept its cache in its request method closure, would that be enough?

Might clean up a lot of the changes we'd have to do to migrate. If we're worried about having to introduce it later, maybe we could ensure all computed artifacts take an inputs object as first argument (like it works now) and an optional context second for future proofing?

patrickhulce · 2018-09-21T14:40:17Z

lighthouse-core/gather/computed/manifest-values.js

-    return 'ManifestValues';
-  }
-
+class ManifestValues {


I believe someone once told me that he thought it was silly we made classes with all static members instead of plain objects ;)

FWIW I kinda like them as classes for the flashy require name consistency, but I was mostly persuaded back then for plain objects, thoughts?

I believe someone once told me that he thought it was silly we made classes with all static members instead of plain objects ;)

haha, I didn't even think about it as I changed things. There used to be some tsc benefit due to polymorphic this (but not an issue with this implementation) and then from augmenting things in javascript passed out on module.exports (but that's all fixed in 3.1).

I'll admit to stockholm syndrome in that I don't see it as ugly and unnecessary any more, but I also don't have any problem with a plain object. The main thing might be the name property, which I think we still want, whether explicit or from just the StaticClass.name default from declaring a class.

true true, this WFM :)

brendankenny · 2018-09-21T18:33:31Z

Just thinking out loud here, what are the primary benefits of attaching the cache to context? If each computed artifact just kept its cache in its request method closure, would that be enough?

I don't think we can do that because of GC. It would be node-module-only (I believe all the other clients destroy the world after every run), but since we use artifacts as the keys, artifacts will keep accumulating from each run.

Good use for WeakMap, but we don't always have unique parent objects for sets of artifacts, so, I don't think that will work. I can't think of another approach around that.

FWIW, I don't think the churn will end up being too bad outside of tests. Basically add a context param to each audit and pass it to each computed artifact request. We also don't have that many computed artifacts when it comes down to it.

Tests need more changes since we make so many calls, but I could reduce the noise there (e.g. just pass in {computedCache: new Map()} rather than doing a getMockContext() call).

patrickhulce · 2018-09-21T21:12:12Z

I don't think we can do that because of GC. It would be node-module-only (I believe all the other clients destroy the world after every run), but since we use artifacts as the keys, artifacts will keep accumulating from each run.

Yeah the clearing mechanism would be ugly at best, if you foresee little pain outside tests, then this makes sense to me 👍

patrickhulce

exciting 🚗ahead! woohoo!

paulirish · 2018-09-21T21:32:02Z

lighthouse-core/runner.js

@@ -269,8 +275,7 @@ class Runner {
      const auditOptions = Object.assign({}, audit.defaultOptions, auditDefn.options);
      const auditContext = {
        options: auditOptions,
-        settings,
-        LighthouseRunWarnings: runWarnings,
+        ...auditsContext,


dont love these two diff objs with such similar names
how about sharedAuditsContext for this one?

paulirish · 2018-09-21T21:34:59Z

lighthouse-core/runner.js

+
+      // Computed artifacts switching to the new system.
+      'new-computed-artifact.js',
+      'manifest-values.js',


This fn is for the gulpfile. wont browserify already discover this manifest-values module without being added manually?

the diff obscures it, but this is the list of filenamesToSkip :)

brendankenny · 2018-09-21T22:08:56Z

lighthouse-core/audits/splash-screen.js

    /** @type {Array<string>} */
    const failures = [];

-    return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
-      SplashScreen.assessManifest(manifestValues, failures);
+    const manifestValues = await ManifestValues.request(context, artifacts.Manifest);


I'll refrain from going promises->async/await in future changes to cut down the diffs as well :)

brendankenny requested review from patrickhulce and paulirish as code owners August 24, 2018 22:54

brendankenny force-pushed the computed branch from 3f2facf to 185bb2a Compare August 24, 2018 23:15

paulirish added the chillin ❄ label Sep 5, 2018

brendankenny force-pushed the computed branch 2 times, most recently from d8e6372 to 6dc2457 Compare September 10, 2018 21:25

brendankenny removed the chillin ❄ label Sep 19, 2018

brendankenny force-pushed the computed branch from e49cb12 to f2817dd Compare September 19, 2018 19:20

patrickhulce reviewed Sep 21, 2018

View reviewed changes

patrickhulce approved these changes Sep 21, 2018

View reviewed changes

paulirish requested changes Sep 21, 2018

View reviewed changes

paulirish reviewed Sep 21, 2018

View reviewed changes

brendankenny added 4 commits September 21, 2018 15:03

core: initial refactor of computedArtifact import/caching

6a2a3a7

switcheroo

30077ef

new form

63a7236

simpler

6b10f70

brendankenny force-pushed the computed branch from f2817dd to 6b10f70 Compare September 21, 2018 22:03

sharing is caring

aba09e9

brendankenny commented Sep 21, 2018

View reviewed changes

paulirish approved these changes Sep 21, 2018

View reviewed changes

brendankenny merged commit 489da22 into master Sep 21, 2018

brendankenny deleted the computed branch September 21, 2018 22:29

brendankenny mentioned this pull request Oct 1, 2018

core(computed): fix new computed artifact interface #6151

Merged

brendankenny mentioned this pull request Oct 10, 2018

core: convert computed artifact loading to regular require() #6204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: initial refactor of computedArtifact import/caching #5907

core: initial refactor of computedArtifact import/caching #5907

brendankenny commented Aug 24, 2018

brendankenny commented Aug 24, 2018

brendankenny commented Aug 24, 2018

patrickhulce commented Aug 25, 2018

brendankenny commented Aug 25, 2018

brendankenny commented Sep 11, 2018

patrickhulce left a comment

patrickhulce Sep 21, 2018

brendankenny Sep 21, 2018

patrickhulce Sep 21, 2018

brendankenny commented Sep 21, 2018

patrickhulce commented Sep 21, 2018

patrickhulce left a comment

paulirish Sep 21, 2018

brendankenny Sep 21, 2018

brendankenny Sep 21, 2018

paulirish Sep 21, 2018

patrickhulce Sep 21, 2018

brendankenny Sep 21, 2018

core: initial refactor of computedArtifact import/caching #5907

core: initial refactor of computedArtifact import/caching #5907

Conversation

brendankenny commented Aug 24, 2018

brendankenny commented Aug 24, 2018

brendankenny commented Aug 24, 2018

patrickhulce commented Aug 25, 2018

brendankenny commented Aug 25, 2018

brendankenny commented Sep 11, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Sep 21, 2018

patrickhulce commented Sep 21, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment