core(computed-artifact): use deep equality over strict #4409
Conversation
patrickhulce
changed the title
| assert.equal(map.get('foo'), 4); | ||
| }); | ||
|
|
||
| it('is not hella slow', () => { |
There was a problem hiding this comment.
wanna set a custom .timeout(x) on this it?
lighthouse-core/lib/custom-map.js
Outdated
| } | ||
|
|
||
| static deepEquals(objA, objB, maxDepth = 5) { | ||
| if (objA === objB) return true; |
There was a problem hiding this comment.
not crazy about maintaining this, even though is VERY well tested. :)
how about we try/catch assert.deepStrictEqual and use that? And we can use our assertTraceEventsEqual thats in asset-saver-test.
There was a problem hiding this comment.
discussed offline, basically exceptions are a dog, the "is not hella slow" test takes 55s a.k.a it's hella slow so we'll just use jdalton's isEqual from lodash instead :)
| class ComputedArtifact { | ||
| /** | ||
| * @param {!ComputedArtifacts} allComputedArtifacts | ||
| */ | ||
| constructor(allComputedArtifacts) { | ||
| /** @private {!Map} */ | ||
| this._cache = new Map(); | ||
| this._cache = new CustomMap(CustomMap.deepEquals); |
There was a problem hiding this comment.
instead of passing it into the constructor i'm thinking it might be helpful to clarify the intent of the arg.
how about:
this._cache = new CustomMap();
this._cache.setEqualityFn(CustomMap.deepEquals);without it being set, it's just using a === b
| @@ -5,13 +5,15 @@ | |||
| */ | |||
| 'use strict'; | |||
|
|
|||
| const CustomMap = require('../../lib/custom-map'); | |||
There was a problem hiding this comment.
this impl doesn't support new CustomMap([[x, 1], [y, 2]]).
given that maybe call it CacheMap? really anything to give it some distance appearing as a true subclass of Map
There was a problem hiding this comment.
ah yeah, good call I'll add support :)
There was a problem hiding this comment.
hmmm. I don't think we have a real usecase for supporting that constructor API. can we keep it out of scope?
There was a problem hiding this comment.
alright alright, cache map it is
|
It's a really stupid question probably but why does this help us for project Lantern? Which use case do we want to tackle here? (not sure why we need deep equal yet) |
not stupid question, it's hard/impossible to tell from just this PR :) basically simulator.js accepts options for what type of connection/cpu/etc to simulate but there's no mechanism for a LH user to control those options, audit/gatherer options gives us this however, because computed artifacts cache their contents based on strict equality the options objects won't match even if they all request lantern on 3G and so the entire graph would be re-computed for every performance opportunity, introducing caching based on deep equality allows us to maintain the status quo computing the graph just once per setting. make sense? |
lighthouse-core/lib/cache-map.js
Outdated
| * It is not meant to be performant and is well-suited to use cases where the number of entries is | ||
| * likely to be small (like computed artifacts). | ||
| */ | ||
| module.exports = class CacheMap { |
There was a problem hiding this comment.
for the record, I'm still not a huge fan of this name, something like ArbitraryEqualityMap would make more sense 😛
There was a problem hiding this comment.
I'm fine with calling it ArbitraryEqualityMap.
Yes but where does the cpu options come in? Do we store it in the computed artifacts? |
Not 100% sure what you're asking, but each audit will have default options that can be overridden from config. Audit will pass the blended final options to the computed artifact and if it hasn't been requested with the same options before will compute it otherwise return the corresponding one from cache. |
| class ComputedArtifact { | ||
| /** | ||
| * @param {!ComputedArtifacts} allComputedArtifacts | ||
| */ | ||
| constructor(allComputedArtifacts) { | ||
| /** @private {!Map} */ | ||
| this._cache = new Map(); | ||
| this._cache = new CacheMap(); | ||
| this._cache.setEqualsFn(CacheMap.deepEquals); |
|
lgtm just waiting for CI |
needed by #3618 and the introduction of audit options (multiple different audits will request the same lantern simulation settings for example)