Bug 219628

Summary: Add max age for a root to be reused.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: dewei_zhu
Status: RESOLVED FIXED    
Severity: Normal CC: dewei_zhu, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rniwa: review+

Description dewei_zhu 2020-12-07 21:20:48 PST
Add max age for a root to be reused.
Comment 1 dewei_zhu 2020-12-07 21:33:14 PST
Created attachment 415611 [details]
Patch
Comment 2 Ryosuke Niwa 2020-12-07 21:45:44 PST
Comment on attachment 415611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415611&action=review

> Websites/perf.webkit.org/public/include/manifest-generator.php:53
> +            'maxReuseRootAgeInDays' => config('maxReuseRootAgeInDays'),

max*RootReuse*AgeInDays

> Websites/perf.webkit.org/public/v3/models/build-request.js:100
> +        const earliestAllowRootCreationDate = rawManifest.maxReuseRootAgeInDays ?
> +            Date.now() - rawManifest.maxReuseRootAgeInDays * 24 * 3600 * 1000 : 0;

I'd call this earliestRootCreatingTimeForReuse

> Websites/perf.webkit.org/public/v3/models/commit-set.js:77
> -    areAllRootsAvailable()
> +    areAllRootsAvailableAndNewerThan(earliestCreationDate)

I don't think it's necessary to call it "andNewerThan".
Also, I'd call the new argument earliestCreationTime instead.

> Websites/perf.webkit.org/public/v3/models/manifest.js:25
>      static fetch()

Why don't we make this async as well?

> Websites/perf.webkit.org/public/v3/models/manifest.js:34
> +            return await RemoteAPI.getJSON('/data/manifest.json');

Neat.

> Websites/perf.webkit.org/public/v3/models/manifest.js:35
> +        } catch(error) {

We shouldn't swallow all errors though. Just 404.
We should probably add a test for that.

> Websites/perf.webkit.org/unit-tests/build-request-tests.js:470
> +        it('should not reuse a root that older than "maxReuseRootAge"', async () => {

that *is* older than.
Comment 3 dewei_zhu 2020-12-09 15:13:07 PST
Comment on attachment 415611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415611&action=review

>> Websites/perf.webkit.org/public/v3/models/manifest.js:35
>> +        } catch(error) {
> 
> We shouldn't swallow all errors though. Just 404.
> We should probably add a test for that.

Added in the change.
Comment 4 dewei_zhu 2020-12-09 15:14:15 PST
Landed in r270607.