Add cache for CommitLog objects to avoid refetching same commit.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Nov 2018 07:36:34 +0000 (07:36 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Nov 2018 07:36:34 +0000 (07:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191621

Reviewed by Ryosuke Niwa.

Added a cache for fully fetched commit log objects to avoid refetching.

* public/v3/models/commit-log.js:
(CommitLog): Added assertion for id.
Removed unused 'remoteId' as it has been removed since r198479.
(CommitLog.async.fetchBetweenRevisions): Turned it into async function.
(CommitLog.async.fetchForSingleRevision): Added the logic to check cache before fetching.
(CommitLog._constructFromRawData): Added logic to add entries to cache.
* public/v3/models/commit-set.js: Fixed measurement set not passing commit id while constructing a
commit log object.
* public/v3/models/repository.js: Added the ability to track fetched commit for certain repository.
(Repository.commitForRevision): Returns a commit for given revision.
(Repository.setCommitForRevision): Sets commit for a given revision.
* unit-tests/commit-log-tests.js: Added unit tests for this change.
Fixed existing tests.
* unit-tests/commit-set-range-bisector-tests.js: Fixed unit tests.
* unit-tests/commit-set-tests.js: Fixed unit tests.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238167 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/models/commit-log.js
Websites/perf.webkit.org/public/v3/models/commit-set.js
Websites/perf.webkit.org/public/v3/models/repository.js
Websites/perf.webkit.org/unit-tests/commit-log-tests.js
Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js
Websites/perf.webkit.org/unit-tests/commit-set-tests.js

index 40519bd..1e00c18 100644 (file)
@@ -1,3 +1,28 @@
+2018-11-13  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Add cache for CommitLog objects to avoid refetching same commit.
+        https://bugs.webkit.org/show_bug.cgi?id=191621
+
+        Reviewed by Ryosuke Niwa.
+
+        Added a cache for fully fetched commit log objects to avoid refetching.
+
+        * public/v3/models/commit-log.js:
+        (CommitLog): Added assertion for id.
+        Removed unused 'remoteId' as it has been removed since r198479.
+        (CommitLog.async.fetchBetweenRevisions): Turned it into async function.
+        (CommitLog.async.fetchForSingleRevision): Added the logic to check cache before fetching.
+        (CommitLog._constructFromRawData): Added logic to add entries to cache.
+        * public/v3/models/repository.js: Added the ability to track fetched commit for certain repository.
+        (Repository.commitForRevision): Fixed measurement set not passing commit id while constructing a
+        commit log object.
+        (Repository.setCommitForRevision): Sets commit for a given revision.
+        * public/v3/models/commit-set.js: Fixed unit tests.
+        * unit-tests/commit-log-tests.js: Added unit tests for this change.
+        Fixed existing tests.
+        * unit-tests/commit-set-range-bisector-tests.js: Fixed unit tests.
+        * unit-tests/commit-set-tests.js: Fixed unit tests.
+
 2018-11-08  Dewei Zhu  <dewei_zhu@apple.com>
 
         commit time returned by '/api/measurement-set' should match the one returned by '/api/commits'.
index c2db63b..130cb90 100644 (file)
@@ -5,12 +5,10 @@ class CommitLog extends DataModelObject {
     {
         console.assert(parseInt(id) == id);
         super(id);
+        console.assert(id == rawData.id)
         this._repository = rawData.repository;
         console.assert(this._repository instanceof Repository);
         this._rawData = rawData;
-        this._remoteId = rawData.id;
-        if (this._remoteId)
-            this.ensureNamedStaticMap('remoteId')[this._remoteId] = this;
         this._ownedCommits = null;
         this._ownerCommit = null;
         this._ownedCommitByOwnedRepository = new Map;
@@ -163,26 +161,33 @@ class CommitLog extends DataModelObject {
         return difference;
     }
 
-    static fetchBetweenRevisions(repository, precedingRevision, lastRevision)
+    static async fetchBetweenRevisions(repository, precedingRevision, lastRevision)
     {
         // FIXME: The cache should be smarter about fetching a range within an already fetched range, etc...
-        // FIXME: We should evict some entires from the cache in cachedFetch.
-        return this.cachedFetch(`/api/commits/${repository.id()}/`, {precedingRevision, lastRevision})
-            .then((data) => this._constructFromRawData(data));
+        // FIXME: We should evict some entries from the cache in cachedFetch.
+        const data = await this.cachedFetch(`/api/commits/${repository.id()}/`, {precedingRevision, lastRevision});
+        return this._constructFromRawData(data);
     }
 
-    static fetchForSingleRevision(repository, revision)
+    static async fetchForSingleRevision(repository, revision)
     {
-        return this.cachedFetch(`/api/commits/${repository.id()}/${revision}`).then((data) => {
-            return this._constructFromRawData(data);
-        });
+        const commit = repository.commitForRevision(revision);
+        if (commit)
+            return [commit];
+
+        const data = await this.cachedFetch(`/api/commits/${repository.id()}/${revision}`);
+        return this._constructFromRawData(data);
     }
 
     static _constructFromRawData(data)
     {
         return data['commits'].map((rawData) => {
-            rawData.repository = Repository.findById(rawData.repository);
-            return CommitLog.ensureSingleton(rawData.id, rawData);
+            const repositoryId = rawData.repository;
+            const repository = Repository.findById(repositoryId);
+            rawData.repository = repository;
+            const commit = CommitLog.ensureSingleton(rawData.id, rawData);
+            repository.setCommitForRevision(commit.revision(), commit);
+            return commit;
         });
     }
 }
index 29a9825..94acc53 100644 (file)
@@ -281,7 +281,7 @@ class MeasurementCommitSet extends CommitSet {
                 continue;
 
             // FIXME: Add a flag to remember the fact this commit log is incomplete.
-            const commit = CommitLog.ensureSingleton(commitId, {repository, revision, order, time});
+            const commit = CommitLog.ensureSingleton(commitId, {id: commitId, repository, revision, order, time});
             this._repositoryToCommitMap.set(repository, commit);
             this._repositories.push(repository);
         }
index 089f62e..9319899 100644 (file)
@@ -8,6 +8,7 @@ class Repository extends LabeledObject {
         this._blameUrl = object.blameUrl;
         this._hasReportedCommits = object.hasReportedCommits;
         this._ownerId = object.owner;
+        this._commitByRevision = new Map;
 
         if (!this._ownerId)
             this.ensureNamedStaticMap('topLevelName')[this.name()] = this;
@@ -26,6 +27,8 @@ class Repository extends LabeledObject {
         return map ? map[name] : null;
     }
 
+    commitForRevision(revision) { return this._commitByRevision.get(revision); }
+    setCommitForRevision(revision, commit) { this._commitByRevision.set(revision, commit); }
     hasUrlForRevision() { return !!this._url; }
 
     urlForRevision(currentRevision)
index 2ed1d80..5f88524 100644 (file)
@@ -10,6 +10,7 @@ const MockRemoteAPI = require('../unit-tests/resources/mock-remote-api.js').Mock
 function webkitCommit()
 {
     return new CommitLog(1, {
+        id: 1,
         repository: MockModels.webkit,
         revision: '200805',
         time: +(new Date('2016-05-13T00:55:57.841344Z')),
@@ -19,6 +20,7 @@ function webkitCommit()
 function oldWebKitCommit()
 {
     return new CommitLog(2, {
+        id: 2,
         repository: MockModels.webkit,
         revision: '200574',
         time: +(new Date('2016-05-09T14:59:23.553767Z')),
@@ -28,6 +30,7 @@ function oldWebKitCommit()
 function gitWebKitCommit()
 {
     return new CommitLog(3, {
+        id: 3,
         repository: MockModels.webkit,
         revision: '6f8b0dbbda95a440503b88db1dd03dad3a7b07fb',
         time: +(new Date('2016-05-13T00:55:57.841344Z')),
@@ -37,6 +40,7 @@ function gitWebKitCommit()
 function oldGitWebKitCommit()
 {
     return new CommitLog(4, {
+        id: 4,
         repository: MockModels.webkit,
         revision: 'ffda14e6db0746d10d0f050907e4a7325851e502',
         time: +(new Date('2016-05-09T14:59:23.553767Z')),
@@ -46,6 +50,7 @@ function oldGitWebKitCommit()
 function osxCommit()
 {
     return new CommitLog(5, {
+        id: 5,
         repository: MockModels.osx,
         revision: '10.11.4 15E65',
         time: null,
@@ -56,6 +61,7 @@ function osxCommit()
 function oldOSXCommit()
 {
     return new CommitLog(6, {
+        id: 6,
         repository: MockModels.osx,
         revision: '10.11.3 15D21',
         time: null,
@@ -66,6 +72,7 @@ function oldOSXCommit()
 function commitWithoutOwnedCommits()
 {
     return new CommitLog(6, {
+        id: 6,
         repository: MockModels.ownerRepository,
         revision: '10.11.4 15E66',
         ownsCommits: false,
@@ -77,6 +84,7 @@ function commitWithoutOwnedCommits()
 function ownerCommit()
 {
     return new CommitLog(5, {
+        id: 5,
         repository: MockModels.ownerRepository,
         revision: '10.11.4 15E65',
         ownsCommits: true,
@@ -88,6 +96,7 @@ function ownerCommit()
 function otherOwnerCommit()
 {
     return new CommitLog(5, {
+        id: 5,
         repository: MockModels.ownerRepository,
         revision: '10.11.4 15E66',
         ownsCommits: true,
@@ -99,6 +108,7 @@ function otherOwnerCommit()
 function ownedCommit()
 {
     return new CommitLog(11, {
+        id: 11,
         repository: MockModels.ownedRepository,
         revision: 'owned-commit-0',
         ownsCommits: true,
@@ -109,6 +119,7 @@ function ownedCommit()
 function anotherOwnedCommit()
 {
     return new CommitLog(11, {
+        id: 11,
         repository: MockModels.ownedRepository,
         revision: 'owned-commit-1',
         ownsCommits: true,
@@ -405,4 +416,109 @@ describe('CommitLog', function () {
 
         });
     });
+
+    const commitsAPIResponse = {
+        "commits":[{
+            "id": "831151",
+            "revision": "236643",
+            "repository": "11",
+            "previousCommit": null,
+            "time": 1538223077403,
+            "order": null,
+            "authorName": "Commit Queue",
+            "authorEmail": "commit-queue@webkit.org",
+            "message": "message",
+            "ownsCommits": false
+        }],
+        "status":"OK"
+    };
+
+    const commitsAPIResponseForOwnedWebKit = {
+        "commits":[{
+            "id": "1831151",
+            "revision": "236643",
+            "repository": "191",
+            "previousCommit": null,
+            "time": 1538223077403,
+            "order": null,
+            "authorName": "Commit Queue",
+            "authorEmail": "commit-queue@webkit.org",
+            "message": "message",
+            "ownsCommits": false
+        }],
+        "status":"OK"
+    };
+
+    describe('fetchForSingleRevision', () => {
+        beforeEach(() => {
+            MockRemoteAPI.reset();
+        });
+
+        it('should avoid fetching same revision from same repository twice', async () => {
+            let fetchingPromise = CommitLog.fetchForSingleRevision(MockModels.webkit, '236643');
+
+            const requests = MockRemoteAPI.requests;
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.webkit.id()}/236643`);
+
+            requests[0].resolve(commitsAPIResponse);
+            const commits = await fetchingPromise;
+            assert.equal(commits.length, 1);
+
+            MockRemoteAPI.reset();
+            assert.equal(requests.length, 0);
+            fetchingPromise = CommitLog.fetchForSingleRevision(MockModels.webkit, '236643');
+            assert.equal(requests.length, 0);
+            const newCommits = await fetchingPromise;
+            assert.equal(newCommits.length, 1);
+
+            assert.equal(commits[0], newCommits[0]);
+        });
+
+        it('should avoid fetching same revision from same repository again if it has been fetched by "fetchBetweenRevisions" before', async () => {
+            let fetchingPromise = CommitLog.fetchBetweenRevisions(MockModels.webkit, '236642', '236643');
+
+            const requests = MockRemoteAPI.requests;
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.webkit.id()}/?precedingRevision=236642&lastRevision=236643`);
+
+            requests[0].resolve(commitsAPIResponse);
+            const commits = await fetchingPromise;
+            assert.equal(commits.length, 1);
+
+            MockRemoteAPI.reset();
+            assert.equal(requests.length, 0);
+            fetchingPromise = CommitLog.fetchForSingleRevision(MockModels.webkit, '236643');
+            assert.equal(requests.length, 0);
+            const newCommits = await fetchingPromise;
+            assert.equal(newCommits.length, 1);
+
+            assert.equal(commits[0], newCommits[0]);
+        });
+
+        it('should not overwrite cache for commits with same revision but from different repositories', async () => {
+            let fetchingPromise = CommitLog.fetchForSingleRevision(MockModels.webkit, '236643');
+
+            const requests = MockRemoteAPI.requests;
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.webkit.id()}/236643`);
+
+            requests[0].resolve(commitsAPIResponse);
+            const commits = await fetchingPromise;
+            assert.equal(commits.length, 1);
+
+            MockRemoteAPI.reset();
+            assert.equal(requests.length, 0);
+
+            fetchingPromise = CommitLog.fetchForSingleRevision(MockModels.ownedWebkit, '236643');
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, `/api/commits/${MockModels.ownedWebkit.id()}/236643`);
+
+            requests[0].resolve(commitsAPIResponseForOwnedWebKit);
+            const newCommits = await fetchingPromise;
+            assert.equal(newCommits.length, 1);
+
+            assert.notEqual(commits[0], newCommits[0]);
+        });
+    });
 });
index 71cb13f..d097b8b 100644 (file)
@@ -12,6 +12,7 @@ describe('CommitSetRangeBisector', () => {
     function makeCommit(id, repository, revision, time, order)
     {
         return CommitLog.ensureSingleton(id, {
+            id,
             repository,
             revision,
             ownsCommits: false,
index 7e39ab7..39fccc5 100644 (file)
@@ -74,6 +74,7 @@ function customCommitSetWithOwnedRepositoryHasSameNameAsNotOwnedRepository()
 function ownerCommit()
 {
     return CommitLog.ensureSingleton(5, {
+        id: 5,
         repository: MockModels.ownerRepository,
         revision: 'owner-commit-0',
         ownsCommits: true,
@@ -84,6 +85,7 @@ function ownerCommit()
 function partialOwnerCommit()
 {
     return CommitLog.ensureSingleton(5, {
+        id: 5,
         repository: MockModels.ownerRepository,
         revision: 'owner-commit-0',
         ownsCommits: null,
@@ -94,6 +96,7 @@ function partialOwnerCommit()
 function ownedCommit()
 {
     return new CommitLog(6, {
+        id: 6,
         repository: MockModels.ownedRepository,
         revision: 'owned-commit-0',
         ownsCommits: null,
@@ -104,6 +107,7 @@ function ownedCommit()
 function webkitCommit()
 {
     return CommitLog.ensureSingleton(2017, {
+        id: 2017,
         repository: MockModels.webkit,
         revision: 'webkit-commit-0',
         ownsCommits: false,
@@ -114,6 +118,7 @@ function webkitCommit()
 function anotherWebKitCommit()
 {
     return CommitLog.ensureSingleton(2018, {
+        id: 2018,
         repository: MockModels.webkit,
         revision: 'webkit-commit-1',
         ownsCommits: false,
@@ -124,6 +129,7 @@ function anotherWebKitCommit()
 function commitWithSVNRevision()
 {
     return CommitLog.ensureSingleton(2019, {
+        id: 2019,
         repository: MockModels.webkit,
         revision: '12345',
         ownsCommits: false,
@@ -134,6 +140,7 @@ function commitWithSVNRevision()
 function anotherCommitWithSVNRevision()
 {
     return CommitLog.ensureSingleton(2020, {
+        id: 2020,
         repository: MockModels.webkit,
         revision: '45678',
         ownsCommits: false,
@@ -144,6 +151,7 @@ function anotherCommitWithSVNRevision()
 function commitWithGitRevision()
 {
     return CommitLog.ensureSingleton(2021, {
+        id: 2021,
         repository: MockModels.webkitGit,
         revision: '13a0590d34f26fda3953c42ff833132a1a6f6f5a',
         ownsCommits: false,
@@ -154,6 +162,7 @@ function commitWithGitRevision()
 function anotherCommitWithGitRevision()
 {
     return CommitLog.ensureSingleton(2022, {
+        id: 2022,
         repository: MockModels.webkitGit,
         revision: '2f8dd3321d4f51c04f4e2019428ce9ffe97f1ef1',
         ownsCommits: false,
@@ -447,7 +456,7 @@ describe('IntermediateCommitSet', () => {
             const owned = ownedCommit();
 
             const commitSet = CommitSet.ensureSingleton('53246456', {revisionItems: [{commit}, {commit: owned, ownerCommit: commit}]});
-            const intermediateCommitSet =new IntermediateCommitSet(commitSet);
+            const intermediateCommitSet = new IntermediateCommitSet(commitSet);
             const fetchingPromise = intermediateCommitSet.fetchCommitLogs();
 
             const requests = MockRemoteAPI.requests;
@@ -459,14 +468,14 @@ describe('IntermediateCommitSet', () => {
 
             requests[0].resolve({commits: [{
                 id: 5,
-                repository: MockModels.ownerRepository,
+                repository: MockModels.ownerRepository.id(),
                 revision: 'owner-commit-0',
                 ownsCommits: true,
                 time: +(new Date('2016-05-13T00:55:57.841344Z')),
             }]});
             requests[1].resolve({commits: [{
                 id: 6,
-                repository: MockModels.ownedRepository,
+                repository: MockModels.ownedRepository.id(),
                 revision: 'owned-commit-0',
                 ownsCommits: false,
                 time: 1456932774000,