From cc8057b7dc46f230c2ceb77e92603fdc057381ce Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Fri, 30 Oct 2020 17:16:44 -0600
Subject: [PATCH 1/4] Add performance instrumentation helpers to MR Diffs App

---
 app/assets/javascripts/diffs/constants.js     |  5 ++
 .../javascripts/diffs/utils/performance.js    | 80 +++++++++++++++++++
 .../javascripts/performance/constants.js      | 14 ++++
 3 files changed, 99 insertions(+)
 create mode 100644 app/assets/javascripts/diffs/utils/performance.js

diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js
index 709bfe693e63b0..79f8c08e389d01 100644
--- a/app/assets/javascripts/diffs/constants.js
+++ b/app/assets/javascripts/diffs/constants.js
@@ -98,3 +98,8 @@ export const RENAMED_DIFF_TRANSITIONS = {
 
 // MR Diffs known events
 export const EVT_EXPAND_ALL_FILES = 'mr:diffs:expandAllFiles';
+export const EVT_PERF_MARK_FILE_TREE_START = 'mr:diffs:perf:fileTreeStart';
+export const EVT_PERF_MARK_FILE_TREE_END = 'mr:diffs:perf:fileTreeEnd';
+export const EVT_PERF_MARK_DIFF_FILES_START = 'mr:diffs:perf:filesStart';
+export const EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN = 'mr:diffs:perf:firstFileShown';
+export const EVT_PERF_MARK_DIFF_FILES_END = 'mr:diffs:perf:filesEnd';
diff --git a/app/assets/javascripts/diffs/utils/performance.js b/app/assets/javascripts/diffs/utils/performance.js
new file mode 100644
index 00000000000000..4946af56137c47
--- /dev/null
+++ b/app/assets/javascripts/diffs/utils/performance.js
@@ -0,0 +1,80 @@
+import { performanceMarkAndMeasure } from '~/performance_utils';
+import {
+  MR_DIFFS_MARK_FILE_TREE_START,
+  MR_DIFFS_MARK_FILE_TREE_END,
+  MR_DIFFS_MARK_DIFF_FILES_START,
+  MR_DIFFS_MARK_FIRST_DIFF_FILE_SHOWN,
+  MR_DIFFS_MARK_DIFF_FILES_END,
+  MR_DIFFS_MEASURE_FILE_TREE_DONE,
+  MR_DIFFS_MEASURE_DIFF_FILES_DONE,
+} from '../../performance_constants';
+
+import eventHub from '../event_hub';
+import {
+  EVT_PERF_MARK_FILE_TREE_START,
+  EVT_PERF_MARK_FILE_TREE_END,
+  EVT_PERF_MARK_DIFF_FILES_START,
+  EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN,
+  EVT_PERF_MARK_DIFF_FILES_END,
+} from '../constants';
+
+function treeStart() {
+  performanceMarkAndMeasure({
+    mark: MR_DIFFS_MARK_FILE_TREE_START,
+  });
+}
+
+function treeEnd() {
+  performanceMarkAndMeasure({
+    mark: MR_DIFFS_MARK_FILE_TREE_END,
+    measures: [
+      {
+        name: MR_DIFFS_MEASURE_FILE_TREE_DONE,
+        start: MR_DIFFS_MARK_FILE_TREE_START,
+        end: MR_DIFFS_MARK_FILE_TREE_END,
+      },
+    ],
+  });
+}
+
+function filesStart() {
+  performanceMarkAndMeasure({
+    mark: MR_DIFFS_MARK_DIFF_FILES_START,
+  });
+}
+
+function filesEnd() {
+  performanceMarkAndMeasure({
+    mark: MR_DIFFS_MARK_DIFF_FILES_END,
+    measures: [
+      {
+        name: MR_DIFFS_MEASURE_DIFF_FILES_DONE,
+        start: MR_DIFFS_MARK_DIFF_FILES_START,
+        end: MR_DIFFS_MARK_DIFF_FILES_END,
+      },
+    ],
+  });
+}
+
+function firstFile() {
+  performanceMarkAndMeasure({
+    mark: MR_DIFFS_MARK_FIRST_DIFF_FILE_SHOWN,
+  });
+}
+
+export const diffsApp = {
+  instrument() {
+    eventHub.$on(EVT_PERF_MARK_FILE_TREE_START, treeStart);
+    eventHub.$on(EVT_PERF_MARK_FILE_TREE_END, treeEnd);
+    eventHub.$on(EVT_PERF_MARK_DIFF_FILES_START, filesStart);
+    eventHub.$on(EVT_PERF_MARK_DIFF_FILES_END, filesEnd);
+    eventHub.$on(EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN, firstFile);
+  },
+  deinstrument() {
+    eventHub.$off(EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN, firstFile);
+    eventHub.$off(EVT_PERF_MARK_DIFF_FILES_END, filesEnd);
+    eventHub.$off(EVT_PERF_MARK_DIFF_FILES_START, filesStart);
+    eventHub.$off(EVT_PERF_MARK_FILE_TREE_END, treeEnd);
+    eventHub.$off(EVT_PERF_MARK_FILE_TREE_START, treeStart);
+  },
+};
diff --git a/app/assets/javascripts/performance/constants.js b/app/assets/javascripts/performance/constants.js
index 6b6b6f1da40f4f..816eb9b3a66cf9 100644
--- a/app/assets/javascripts/performance/constants.js
+++ b/app/assets/javascripts/performance/constants.js
@@ -29,3 +29,17 @@ export const WEBIDE_MARK_FILE_FINISH = 'webide-file-finished';
 export const WEBIDE_MEASURE_TREE_FROM_REQUEST = 'webide-tree-loading-from-request';
 export const WEBIDE_MEASURE_FILE_FROM_REQUEST = 'webide-file-loading-from-request';
 export const WEBIDE_MEASURE_FILE_AFTER_INTERACTION = 'webide-file-loading-after-interaction';
+
+//
+// MR Diffs namespace
+
+// Marks
+export const MR_DIFFS_MARK_FILE_TREE_START = 'mr-diffs-mark-file-tree-start';
+export const MR_DIFFS_MARK_FILE_TREE_END = 'mr-diffs-mark-file-tree-end';
+export const MR_DIFFS_MARK_DIFF_FILES_START = 'mr-diffs-mark-diff-files-start';
+export const MR_DIFFS_MARK_FIRST_DIFF_FILE_SHOWN = 'mr-diffs-mark-first-diff-file-shown';
+export const MR_DIFFS_MARK_DIFF_FILES_END = 'mr-diffs-mark-diff-files-end';
+
+// Measures
+export const MR_DIFFS_MEASURE_FILE_TREE_DONE = 'mr-diffs-measure-file-tree-done';
+export const MR_DIFFS_MEASURE_DIFF_FILES_DONE = 'mr-diffs-measure-diff-files-done';
-- 
GitLab


From 14b09730ffc6d0c58716071ed7a50503d2b92320 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Fri, 30 Oct 2020 17:23:05 -0600
Subject: [PATCH 2/4] Add performance instrumentation to MR Diffs app

- When adding the diffs event hub, also rename the
existing hub in the actions to notesEventHub
---
 .../javascripts/diffs/components/app.vue      | 12 +++++-
 .../diffs/components/diff_file.vue            | 37 ++++++++++++++++++-
 app/assets/javascripts/diffs/store/actions.js | 19 +++++++---
 .../javascripts/diffs/utils/performance.js    |  4 +-
 4 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index a800cc8edc8131..9d8d184a3f614b 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -20,6 +20,8 @@ import HiddenFilesWarning from './hidden_files_warning.vue';
 import MergeConflictWarning from './merge_conflict_warning.vue';
 import CollapsedFilesWarning from './collapsed_files_warning.vue';
 
+import { diffsApp } from '../utils/performance';
+
 import {
   TREE_LIST_WIDTH_STORAGE_KEY,
   INITIAL_TREE_WIDTH,
@@ -272,8 +274,12 @@ export default {
       );
     }
   },
+  beforeCreate() {
+    diffsApp.instrument();
+  },
   created() {
     this.adjustView();
+
     eventHub.$once('fetchDiffData', this.fetchData);
     eventHub.$on('refetchDiffData', this.refetchDiffData);
     this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
@@ -294,6 +300,8 @@ export default {
     );
   },
   beforeDestroy() {
+    diffsApp.deinstrument();
+
     eventHub.$off('fetchDiffData', this.fetchData);
     eventHub.$off('refetchDiffData', this.refetchDiffData);
     this.removeEventListeners();
@@ -487,9 +495,11 @@ export default {
           <div v-if="isBatchLoading" class="loading"><gl-loading-icon size="lg" /></div>
           <template v-else-if="renderDiffFiles">
             <diff-file
-              v-for="file in diffs"
+              v-for="(file, index) in diffs"
               :key="file.newPath"
               :file="file"
+              :is-first-file="index === 0"
+              :is-last-file="index === diffs.length - 1"
               :help-page-path="helpPagePath"
               :can-current-user-fork="canCurrentUserFork"
               :view-diffs-file-by-file="viewDiffsFileByFile"
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index 27bda194c5c34f..59bd2d9015849b 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -15,6 +15,8 @@ import {
   DIFF_FILE_AUTOMATIC_COLLAPSE,
   DIFF_FILE_MANUAL_COLLAPSE,
   EVT_EXPAND_ALL_FILES,
+  EVT_PERF_MARK_DIFF_FILES_END,
+  EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN,
 } from '../constants';
 import { DIFF_FILE, GENERIC_ERROR } from '../i18n';
 import eventHub from '../event_hub';
@@ -35,6 +37,16 @@ export default {
       type: Object,
       required: true,
     },
+    isFirstFile: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    isLastFile: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
     canCurrentUserFork: {
       type: Boolean,
       required: true,
@@ -160,6 +172,11 @@ export default {
     notesEventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.requestDiff);
     eventHub.$on(EVT_EXPAND_ALL_FILES, this.expandAllListener);
   },
+  async mounted() {
+    if (this.hasDiff) {
+      await this.postRender();
+    }
+  },
   beforeDestroy() {
     eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener);
   },
@@ -175,6 +192,23 @@ export default {
         this.handleToggle();
       }
     },
+    async postRender() {
+      const eventsForThisFile = [];
+
+      if (this.isFirstFile) {
+        eventsForThisFile.push(EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN);
+      }
+
+      if (this.isLastFile) {
+        eventsForThisFile.push(EVT_PERF_MARK_DIFF_FILES_END);
+      }
+
+      await this.$nextTick();
+
+      eventsForThisFile.forEach(event => {
+        eventHub.$emit(event);
+      });
+    },
     handleToggle() {
       const currentCollapsedFlag = this.isCollapsed;
 
@@ -197,7 +231,8 @@ export default {
         })
         .then(() => {
           requestIdleCallback(
-            () => {
+            async () => {
+              await this.postRender();
               this.assignDiscussionsToDiff(this.getDiffFileDiscussions(this.file));
             },
             { timeout: 1000 },
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 5a3d836a1584ed..72b99ca84865fc 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -8,7 +8,8 @@ import { __, s__ } from '~/locale';
 import { handleLocationHash, historyPushState, scrollToElement } from '~/lib/utils/common_utils';
 import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
 import TreeWorker from '../workers/tree_worker';
-import eventHub from '../../notes/event_hub';
+import notesEventHub from '../../notes/event_hub';
+import eventHub from '../event_hub';
 import {
   getDiffPositionByLineCode,
   getNoteFormData,
@@ -42,6 +43,9 @@ import {
   NO_SHOW_WHITESPACE,
   DIFF_FILE_MANUAL_COLLAPSE,
   DIFF_FILE_AUTOMATIC_COLLAPSE,
+  EVT_PERF_MARK_FILE_TREE_START,
+  EVT_PERF_MARK_FILE_TREE_END,
+  EVT_PERF_MARK_DIFF_FILES_START,
 } from '../constants';
 import { diffViewerModes } from '~/ide/constants';
 import { isCollapsed } from '../diff_file';
@@ -78,6 +82,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
 
   commit(types.SET_BATCH_LOADING, true);
   commit(types.SET_RETRIEVING_BATCHES, true);
+  eventHub.$emit(EVT_PERF_MARK_DIFF_FILES_START);
 
   const getBatch = (page = 1) =>
     axios
@@ -139,9 +144,11 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
   };
 
   commit(types.SET_LOADING, true);
+  eventHub.$emit(EVT_PERF_MARK_FILE_TREE_START);
 
   worker.addEventListener('message', ({ data }) => {
     commit(types.SET_TREE_DATA, data);
+    eventHub.$emit(EVT_PERF_MARK_FILE_TREE_END);
 
     worker.terminate();
   });
@@ -215,7 +222,7 @@ export const assignDiscussionsToDiff = (
   }
 
   Vue.nextTick(() => {
-    eventHub.$emit('scrollToDiscussion');
+    notesEventHub.$emit('scrollToDiscussion');
   });
 };
 
@@ -240,7 +247,7 @@ export const renderFileForDiscussionId = ({ commit, rootState, state }, discussi
       }
 
       if (file.viewer.automaticallyCollapsed) {
-        eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
+        notesEventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
         scrollToElement(document.getElementById(file.file_hash));
       } else if (file.viewer.manuallyCollapsed) {
         commit(types.SET_FILE_COLLAPSED, {
@@ -248,9 +255,9 @@ export const renderFileForDiscussionId = ({ commit, rootState, state }, discussi
           collapsed: false,
           trigger: DIFF_FILE_AUTOMATIC_COLLAPSE,
         });
-        eventHub.$emit('scrollToDiscussion');
+        notesEventHub.$emit('scrollToDiscussion');
       } else {
-        eventHub.$emit('scrollToDiscussion');
+        notesEventHub.$emit('scrollToDiscussion');
       }
     }
   }
@@ -485,7 +492,7 @@ export const setShowWhitespace = ({ commit }, { showWhitespace, pushState = fals
     historyPushState(mergeUrlParams({ w }, window.location.href));
   }
 
-  eventHub.$emit('refetchDiffData');
+  notesEventHub.$emit('refetchDiffData');
 };
 
 export const toggleFileFinder = ({ commit }, visible) => {
diff --git a/app/assets/javascripts/diffs/utils/performance.js b/app/assets/javascripts/diffs/utils/performance.js
index 4946af56137c47..dcde6f4ecc49bd 100644
--- a/app/assets/javascripts/diffs/utils/performance.js
+++ b/app/assets/javascripts/diffs/utils/performance.js
@@ -1,4 +1,4 @@
-import { performanceMarkAndMeasure } from '~/performance_utils';
+import { performanceMarkAndMeasure } from '~/performance/utils';
 import {
   MR_DIFFS_MARK_FILE_TREE_START,
   MR_DIFFS_MARK_FILE_TREE_END,
@@ -7,7 +7,7 @@ import {
   MR_DIFFS_MARK_DIFF_FILES_END,
   MR_DIFFS_MEASURE_FILE_TREE_DONE,
   MR_DIFFS_MEASURE_DIFF_FILES_DONE,
-} from '../../performance_constants';
+} from '../../performance/constants';
 
 import eventHub from '../event_hub';
 import {
-- 
GitLab


From 9fbb106205b9a7ca6eb423605bcedcb82c8da4f5 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Fri, 6 Nov 2020 09:31:56 -0700
Subject: [PATCH 3/4] Add changelog for added performance marks

---
 changelogs/unreleased/feature-mr-diffs-performance-marks.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/feature-mr-diffs-performance-marks.yml

diff --git a/changelogs/unreleased/feature-mr-diffs-performance-marks.yml b/changelogs/unreleased/feature-mr-diffs-performance-marks.yml
new file mode 100644
index 00000000000000..914991b2a91c49
--- /dev/null
+++ b/changelogs/unreleased/feature-mr-diffs-performance-marks.yml
@@ -0,0 +1,5 @@
+---
+title: Add performance marks and measures to the MR Diffs app at critical moments
+merge_request: 46434
+author:
+type: other
-- 
GitLab


From 18b60d423ee45068b70e5ec7c92e4365ad00646c Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Fri, 6 Nov 2020 16:49:08 -0700
Subject: [PATCH 4/4] Add tests for the events a diff file might emit

---
 .../diffs/components/diff_file_spec.js        | 78 ++++++++++++++++++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js
index bd21252eb5a384..71e0ffd176f46f 100644
--- a/spec/frontend/diffs/components/diff_file_spec.js
+++ b/spec/frontend/diffs/components/diff_file_spec.js
@@ -2,6 +2,7 @@ import Vuex from 'vuex';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
 
 import createDiffsStore from '~/diffs/store/modules';
+import createNotesStore from '~/notes/stores/modules';
 import diffFileMockDataReadable from '../mock_data/diff_file';
 import diffFileMockDataUnreadable from '../mock_data/diff_file_unreadable';
 
@@ -10,9 +11,13 @@ import DiffFileHeaderComponent from '~/diffs/components/diff_file_header.vue';
 import DiffContentComponent from '~/diffs/components/diff_content.vue';
 
 import eventHub from '~/diffs/event_hub';
+import {
+  EVT_EXPAND_ALL_FILES,
+  EVT_PERF_MARK_DIFF_FILES_END,
+  EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN,
+} from '~/diffs/constants';
 
 import { diffViewerModes, diffViewerErrors } from '~/ide/constants';
-import { EVT_EXPAND_ALL_FILES } from '~/diffs/constants';
 
 function changeViewer(store, index, { automaticallyCollapsed, manuallyCollapsed, name }) {
   const file = store.state.diffs.diffFiles[index];
@@ -58,12 +63,13 @@ function markFileToBeRendered(store, index = 0) {
   });
 }
 
-function createComponent({ file }) {
+function createComponent({ file, first = false, last = false }) {
   const localVue = createLocalVue();
 
   localVue.use(Vuex);
 
   const store = new Vuex.Store({
+    ...createNotesStore(),
     modules: {
       diffs: createDiffsStore(),
     },
@@ -78,6 +84,8 @@ function createComponent({ file }) {
       file,
       canCurrentUserFork: false,
       viewDiffsFileByFile: false,
+      isFirstFile: first,
+      isLastFile: last,
     },
   });
 
@@ -117,6 +125,72 @@ describe('DiffFile', () => {
 
   afterEach(() => {
     wrapper.destroy();
+    wrapper = null;
+  });
+
+  describe('bus events', () => {
+    beforeEach(() => {
+      jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+    });
+
+    describe('during mount', () => {
+      it.each`
+        first    | last     | events                                                                 | file
+        ${false} | ${false} | ${[]}                                                                  | ${{ inlineLines: [], parallelLines: [], readableText: true }}
+        ${true}  | ${true}  | ${[]}                                                                  | ${{ inlineLines: [], parallelLines: [], readableText: true }}
+        ${true}  | ${false} | ${[EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN]}                               | ${false}
+        ${false} | ${true}  | ${[EVT_PERF_MARK_DIFF_FILES_END]}                                      | ${false}
+        ${true}  | ${true}  | ${[EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN, EVT_PERF_MARK_DIFF_FILES_END]} | ${false}
+      `(
+        'emits the events $events based on the file and its position ({ first: $first, last: $last }) among all files',
+        async ({ file, first, last, events }) => {
+          if (file) {
+            forceHasDiff({ store, ...file });
+          }
+
+          ({ wrapper, store } = createComponent({
+            file: store.state.diffs.diffFiles[0],
+            first,
+            last,
+          }));
+
+          await wrapper.vm.$nextTick();
+
+          expect(eventHub.$emit).toHaveBeenCalledTimes(events.length);
+          events.forEach(event => {
+            expect(eventHub.$emit).toHaveBeenCalledWith(event);
+          });
+        },
+      );
+    });
+
+    describe('after loading the diff', () => {
+      it('indicates that it loaded the file', async () => {
+        forceHasDiff({ store, inlineLines: [], parallelLines: [], readableText: true });
+        ({ wrapper, store } = createComponent({
+          file: store.state.diffs.diffFiles[0],
+          first: true,
+          last: true,
+        }));
+
+        jest.spyOn(wrapper.vm, 'loadCollapsedDiff').mockResolvedValue(getReadableFile());
+        jest.spyOn(window, 'requestIdleCallback').mockImplementation(fn => fn());
+
+        makeFileAutomaticallyCollapsed(store);
+
+        await wrapper.vm.$nextTick(); // Wait for store updates to flow into the component
+
+        toggleFile(wrapper);
+
+        await wrapper.vm.$nextTick(); // Wait for the load to resolve
+        await wrapper.vm.$nextTick(); // Wait for the idleCallback
+        await wrapper.vm.$nextTick(); // Wait for nextTick inside postRender
+
+        expect(eventHub.$emit).toHaveBeenCalledTimes(2);
+        expect(eventHub.$emit).toHaveBeenCalledWith(EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN);
+        expect(eventHub.$emit).toHaveBeenCalledWith(EVT_PERF_MARK_DIFF_FILES_END);
+      });
+    });
   });
 
   describe('template', () => {
-- 
GitLab