Bug 191621 – Add cache for CommitLog objects to avoid refetching same commit.

• Component: New Bugs
• Status: RESOLVED FIXED

Add cache for CommitLog objects to avoid refetching same commit.

Created attachment 354760 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354760 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:11
> +        (CommitLog): Added assertion for id. Removed unused 'remoteId'.

Refer to r198479 where the relevant code was removed.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:174
> +        const commit = this.ensureNamedStaticMap(`repository-${repository.id()}`)[revision];

ensureNamedStaticMap shouldn't be called with a dynamically generated name like this because it would be slow.
In general, ensureNamedStaticMap is designed to work with a small fixed number of maps.

Instead, this.ensureNamedStaticMap('repositoryMap') should have a map.
Maybe an even better design is for each instance of a Repository object to have this map
although it's slightly inelegant to reference CommitLog and Repository like that.

Created attachment 354765 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354765 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-log.js:174
> +        const commit = repository.ensureNamedStaticMap('revision')[revision];

Oh, no, that's not what I mean. ensureNamedStaticMap is per class so this is simply an alias to Repository.ensureNamedStaticMap('revision')
You'd have to create your own instance variable on Repository. Please add a test for this.
If you called fetchForSingleRevision on two different repositories, it would override each other with the current patch.

Created attachment 354772 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354772 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:12
> +        Removed unused 'remoteId' has it has been removed since r198479.

s/has it has/as it has/

> Websites/perf.webkit.org/public/v3/models/repository.js:38
> +

No need for a blank line here.

Landed in r238167