Merge branch...

Merge branch '216943-add-performance-marks-and-measures-user-timing-api-to-mr-diffs-for-relevant-moments' into 'master'

Add performance marks and measures to the MR Diffs app

See merge request gitlab-org/gitlab!46434

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"

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 },

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';

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) => {

app/assets/javascripts/diffs/utils/performance.js

+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);
+  },
+};

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';

changelogs/unreleased/feature-mr-diffs-performance-marks.yml

+---
+title: Add performance marks and measures to the MR Diffs app at critical moments
+merge_request: 46434
+author:
+type: other

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', () => {