From c49d4a4985c806aa62dcd4899013143484b5d3c6 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Thu, 7 Mar 2019 09:22:05 +0000 Subject: [PATCH] Improve files_decorator performance **How?** Previously the files_decorator inserted parent folders inefficiently. It started at the first part and ensured each was inserted. Since the file entries are given to use in alphabetical order, we can start at the last part of the blob's parents. If this exists, we can short circuit and be done inserting parents. **What else?** - Improve performance of decorateData. The object spread operator is not needed because the object is brand new. --- app/assets/javascripts/ide/lib/files.js | 113 ++++++++++++++++++ app/assets/javascripts/ide/stores/actions.js | 48 ++++---- .../javascripts/ide/stores/actions/tree.js | 46 +++---- app/assets/javascripts/ide/stores/utils.js | 5 +- .../stores/workers/files_decorator_worker.js | 100 ---------------- ...336-improve-web-ide-launch-performance.yml | 5 + spec/javascripts/ide/lib/files_spec.js | 77 ++++++++++++ .../ide/stores/actions/tree_spec.js | 7 ++ 8 files changed, 249 insertions(+), 152 deletions(-) create mode 100644 app/assets/javascripts/ide/lib/files.js delete mode 100644 app/assets/javascripts/ide/stores/workers/files_decorator_worker.js create mode 100644 changelogs/unreleased/53336-improve-web-ide-launch-performance.yml create mode 100644 spec/javascripts/ide/lib/files_spec.js diff --git a/app/assets/javascripts/ide/lib/files.js b/app/assets/javascripts/ide/lib/files.js new file mode 100644 index 00000000000..5dfba8fe531 --- /dev/null +++ b/app/assets/javascripts/ide/lib/files.js @@ -0,0 +1,113 @@ +import { viewerInformationForPath } from '~/vue_shared/components/content_viewer/lib/viewer_utils'; +import { decorateData, sortTree } from '../stores/utils'; + +export const splitParent = path => { + const idx = path.lastIndexOf('/'); + + return { + parent: idx >= 0 ? path.substring(0, idx) : null, + name: idx >= 0 ? path.substring(idx + 1) : path, + }; +}; + +/** + * Create file objects from a list of file paths. + */ +export const decorateFiles = ({ + data, + projectId, + branchId, + tempFile = false, + content = '', + base64 = false, +}) => { + const treeList = []; + const entries = {}; + + // These mutable variable references end up being exported and used by `createTempEntry` + let file; + let parentPath; + + const insertParent = path => { + if (!path) { + return null; + } else if (entries[path]) { + return entries[path]; + } + + const { parent, name } = splitParent(path); + const parentFolder = parent && insertParent(parent); + parentPath = parentFolder && parentFolder.path; + + const tree = decorateData({ + projectId, + branchId, + id: path, + name, + path, + url: `/${projectId}/tree/${branchId}/-/${path}/`, + type: 'tree', + parentTreeUrl: parentFolder ? parentFolder.url : `/${projectId}/tree/${branchId}/`, + tempFile, + changed: tempFile, + opened: tempFile, + parentPath, + }); + + Object.assign(entries, { + [path]: tree, + }); + + if (parentFolder) { + parentFolder.tree.push(tree); + } else { + treeList.push(tree); + } + + return tree; + }; + + data.forEach(path => { + const { parent, name } = splitParent(path); + + const fileFolder = parent && insertParent(parent); + + if (name) { + parentPath = fileFolder && fileFolder.path; + + file = decorateData({ + projectId, + branchId, + id: path, + name, + path, + url: `/${projectId}/blob/${branchId}/-/${path}`, + type: 'blob', + parentTreeUrl: fileFolder ? fileFolder.url : `/${projectId}/blob/${branchId}`, + tempFile, + changed: tempFile, + content, + base64, + previewMode: viewerInformationForPath(name), + parentPath, + }); + + Object.assign(entries, { + [path]: file, + }); + + if (fileFolder) { + fileFolder.tree.push(file); + } else { + treeList.push(file); + } + } + }); + + return { + entries, + treeList: sortTree(treeList), + file, + parentPath, + }; +}; diff --git a/app/assets/javascripts/ide/stores/actions.js b/app/assets/javascripts/ide/stores/actions.js index 95d91e08757..7b660bda081 100644 --- a/app/assets/javascripts/ide/stores/actions.js +++ b/app/assets/javascripts/ide/stores/actions.js @@ -3,7 +3,7 @@ import Vue from 'vue'; import { visitUrl } from '~/lib/utils/url_utility'; import flash from '~/flash'; import * as types from './mutation_types'; -import FilesDecoratorWorker from './workers/files_decorator_worker'; +import { decorateFiles } from '../lib/files'; import { stageKeys } from '../constants'; export const redirectToUrl = (_, url) => visitUrl(url); @@ -56,7 +56,6 @@ export const createTempEntry = ( { name, type, content = '', base64 = false }, ) => new Promise(resolve => { - const worker = new FilesDecoratorWorker(); const fullName = name.slice(-1) !== '/' && type === 'tree' ? `${name}/` : name; if (state.entries[name]) { @@ -74,31 +73,7 @@ export const createTempEntry = ( return null; } - worker.addEventListener('message', ({ data }) => { - const { file, parentPath } = data; - - worker.terminate(); - - commit(types.CREATE_TMP_ENTRY, { - data, - projectId: state.currentProjectId, - branchId: state.currentBranchId, - }); - - if (type === 'blob') { - commit(types.TOGGLE_FILE_OPEN, file.path); - commit(types.ADD_FILE_TO_CHANGED, file.path); - dispatch('setFileActive', file.path); - } - - if (parentPath && !state.entries[parentPath].opened) { - commit(types.TOGGLE_TREE_OPEN, parentPath); - } - - resolve(file); - }); - - worker.postMessage({ + const data = decorateFiles({ data: [fullName], projectId: state.currentProjectId, branchId: state.currentBranchId, @@ -107,6 +82,25 @@ export const createTempEntry = ( base64, content, }); + const { file, parentPath } = data; + + commit(types.CREATE_TMP_ENTRY, { + data, + projectId: state.currentProjectId, + branchId: state.currentBranchId, + }); + + if (type === 'blob') { + commit(types.TOGGLE_FILE_OPEN, file.path); + commit(types.ADD_FILE_TO_CHANGED, file.path); + dispatch('setFileActive', file.path); + } + + if (parentPath && !state.entries[parentPath].opened) { + commit(types.TOGGLE_TREE_OPEN, parentPath); + } + + resolve(file); return null; }); diff --git a/app/assets/javascripts/ide/stores/actions/tree.js b/app/assets/javascripts/ide/stores/actions/tree.js index de5f6050074..3d83e4a9ba5 100644 --- a/app/assets/javascripts/ide/stores/actions/tree.js +++ b/app/assets/javascripts/ide/stores/actions/tree.js @@ -1,7 +1,8 @@ +import _ from 'underscore'; import { __ } from '../../../locale'; import service from '../../services'; import * as types from '../mutation_types'; -import FilesDecoratorWorker from '../workers/files_decorator_worker'; +import { decorateFiles } from '../../lib/files'; export const toggleTreeOpen = ({ commit }, path) => { commit(types.TOGGLE_TREE_OPEN, path); @@ -32,6 +33,19 @@ export const handleTreeEntryAction = ({ commit, dispatch }, row) => { dispatch('showTreeEntry', row.path); }; +export const setDirectoryData = ({ state, commit }, { projectId, branchId, treeList }) => { + const selectedTree = state.trees[`${projectId}/${branchId}`]; + + commit(types.SET_DIRECTORY_DATA, { + treePath: `${projectId}/${branchId}`, + data: treeList, + }); + commit(types.TOGGLE_LOADING, { + entry: selectedTree, + forceValue: false, + }); +}; + export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } = {}) => new Promise((resolve, reject) => { if ( @@ -45,31 +59,19 @@ export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } = service .getFiles(selectedProject.web_url, branchId) .then(({ data }) => { - const worker = new FilesDecoratorWorker(); - worker.addEventListener('message', e => { - const { entries, treeList } = e.data; - const selectedTree = state.trees[`${projectId}/${branchId}`]; - - commit(types.SET_ENTRIES, entries); - commit(types.SET_DIRECTORY_DATA, { - treePath: `${projectId}/${branchId}`, - data: treeList, - }); - commit(types.TOGGLE_LOADING, { - entry: selectedTree, - forceValue: false, - }); - - worker.terminate(); - - resolve(); - }); - - worker.postMessage({ + const { entries, treeList } = decorateFiles({ data, projectId, branchId, }); + + commit(types.SET_ENTRIES, entries); + + // Defer setting the directory data because this triggers some intense rendering. + // The entries is all we need to load the file editor. + _.defer(() => dispatch('setDirectoryData', { projectId, branchId, treeList })); + + resolve(); }) .catch(e => { if (e.response.status === 404) { diff --git a/app/assets/javascripts/ide/stores/utils.js b/app/assets/javascripts/ide/stores/utils.js index 0ede76fd1e0..0b2a18e9c8a 100644 --- a/app/assets/javascripts/ide/stores/utils.js +++ b/app/assets/javascripts/ide/stores/utils.js @@ -75,8 +75,7 @@ export const decorateData = entity => { parentPath = '', } = entity; - return { - ...dataStructure(), + return Object.assign(dataStructure(), { id, projectId, branchId, @@ -97,7 +96,7 @@ export const decorateData = entity => { file_lock, html, parentPath, - }; + }); }; export const findEntry = (tree, type, name, prop = 'name') => diff --git a/app/assets/javascripts/ide/stores/workers/files_decorator_worker.js b/app/assets/javascripts/ide/stores/workers/files_decorator_worker.js deleted file mode 100644 index fa35c215880..00000000000 --- a/app/assets/javascripts/ide/stores/workers/files_decorator_worker.js +++ /dev/null @@ -1,100 +0,0 @@ -import { viewerInformationForPath } from '~/vue_shared/components/content_viewer/lib/viewer_utils'; -import { decorateData, sortTree } from '../utils'; - -// eslint-disable-next-line no-restricted-globals -self.addEventListener('message', e => { - const { data, projectId, branchId, tempFile = false, content = '', base64 = false } = e.data; - - const treeList = []; - let file; - let parentPath; - const entries = data.reduce((acc, path) => { - const pathSplit = path.split('/'); - const blobName = pathSplit.pop().trim(); - - if (pathSplit.length > 0) { - pathSplit.reduce((pathAcc, folderName) => { - const parentFolder = acc[pathAcc[pathAcc.length - 1]]; - const folderPath = `${parentFolder ? `${parentFolder.path}/` : ''}${folderName}`; - const foundEntry = acc[folderPath]; - - if (!foundEntry) { - parentPath = parentFolder ? parentFolder.path : null; - - const tree = decorateData({ - projectId, - branchId, - id: folderPath, - name: folderName, - path: folderPath, - url: `/${projectId}/tree/${branchId}/-/${folderPath}/`, - type: 'tree', - parentTreeUrl: parentFolder ? parentFolder.url : `/${projectId}/tree/${branchId}/`, - tempFile, - changed: tempFile, - opened: tempFile, - parentPath, - }); - - Object.assign(acc, { - [folderPath]: tree, - }); - - if (parentFolder) { - parentFolder.tree.push(tree); - } else { - treeList.push(tree); - } - - pathAcc.push(tree.path); - } else { - pathAcc.push(foundEntry.path); - } - - return pathAcc; - }, []); - } - - if (blobName !== '') { - const fileFolder = acc[pathSplit.join('/')]; - parentPath = fileFolder ? fileFolder.path : null; - - file = decorateData({ - projectId, - branchId, - id: path, - name: blobName, - path, - url: `/${projectId}/blob/${branchId}/-/${path}`, - type: 'blob', - parentTreeUrl: fileFolder ? fileFolder.url : `/${projectId}/blob/${branchId}`, - tempFile, - changed: tempFile, - content, - base64, - previewMode: viewerInformationForPath(blobName), - parentPath, - }); - - Object.assign(acc, { - [path]: file, - }); - - if (fileFolder) { - fileFolder.tree.push(file); - } else { - treeList.push(file); - } - } - - return acc; - }, {}); - - // eslint-disable-next-line no-restricted-globals - self.postMessage({ - entries, - treeList: sortTree(treeList), - file, - parentPath, - }); -}); diff --git a/changelogs/unreleased/53336-improve-web-ide-launch-performance.yml b/changelogs/unreleased/53336-improve-web-ide-launch-performance.yml new file mode 100644 index 00000000000..65439f5a6c2 --- /dev/null +++ b/changelogs/unreleased/53336-improve-web-ide-launch-performance.yml @@ -0,0 +1,5 @@ +--- +title: Improve Web IDE launch performance +merge_request: 25700 +author: +type: performance diff --git a/spec/javascripts/ide/lib/files_spec.js b/spec/javascripts/ide/lib/files_spec.js new file mode 100644 index 00000000000..fe791aa2b74 --- /dev/null +++ b/spec/javascripts/ide/lib/files_spec.js @@ -0,0 +1,77 @@ +import { viewerInformationForPath } from '~/vue_shared/components/content_viewer/lib/viewer_utils'; +import { decorateFiles, splitParent } from '~/ide/lib/files'; +import { decorateData } from '~/ide/stores/utils'; + +const TEST_BRANCH_ID = 'lorem-ipsum'; +const TEST_PROJECT_ID = 10; + +const createEntries = paths => { + const createEntry = (acc, { path, type, children }) => { + // Sometimes we need to end the url with a '/' + const createUrl = base => (type === 'tree' ? `${base}/` : base); + + const { name, parent } = splitParent(path); + const parentEntry = acc[parent]; + + acc[path] = { + ...decorateData({ + projectId: TEST_PROJECT_ID, + branchId: TEST_BRANCH_ID, + id: path, + name, + path, + url: createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}/-/${path}`), + type, + previewMode: viewerInformationForPath(path), + parentPath: parent, + parentTreeUrl: parentEntry + ? parentEntry.url + : createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}`), + }), + tree: children.map(childName => jasmine.objectContaining({ name: childName })), + }; + + return acc; + }; + + const entries = paths.reduce(createEntry, {}); + + // Wrap entries in jasmine.objectContaining. + // We couldn't do this earlier because we still need to select properties from parent entries. + return Object.keys(entries).reduce((acc, key) => { + acc[key] = jasmine.objectContaining(entries[key]); + + return acc; + }, {}); +}; + +describe('IDE lib decorate files', () => { + it('creates entries and treeList', () => { + const data = ['app/assets/apples/foo.js', 'app/bugs.js', 'README.md']; + const expectedEntries = createEntries([ + { path: 'app', type: 'tree', children: ['assets', 'bugs.js'] }, + { path: 'app/assets', type: 'tree', children: ['apples'] }, + { path: 'app/assets/apples', type: 'tree', children: ['foo.js'] }, + { path: 'app/assets/apples/foo.js', type: 'blob', children: [] }, + { path: 'app/bugs.js', type: 'blob', children: [] }, + { path: 'README.md', type: 'blob', children: [] }, + ]); + + const { entries, treeList } = decorateFiles({ + data, + branchId: TEST_BRANCH_ID, + projectId: TEST_PROJECT_ID, + }); + + // Here we test the keys and then each key/value individually because `expect(entries).toEqual(expectedEntries)` + // was taking a very long time for some reason. Probably due to large objects and nested `jasmine.objectContaining`. + const entryKeys = Object.keys(entries); + + expect(entryKeys).toEqual(Object.keys(expectedEntries)); + entryKeys.forEach(key => { + expect(entries[key]).toEqual(expectedEntries[key]); + }); + + expect(treeList).toEqual([expectedEntries.app, expectedEntries['README.md']]); + }); +}); diff --git a/spec/javascripts/ide/stores/actions/tree_spec.js b/spec/javascripts/ide/stores/actions/tree_spec.js index bd41e87bf0e..fbb676aab33 100644 --- a/spec/javascripts/ide/stores/actions/tree_spec.js +++ b/spec/javascripts/ide/stores/actions/tree_spec.js @@ -20,6 +20,7 @@ describe('Multi-file store tree actions', () => { }; beforeEach(() => { + jasmine.clock().install(); spyOn(router, 'push'); mock = new MockAdapter(axios); @@ -37,6 +38,7 @@ describe('Multi-file store tree actions', () => { }); afterEach(() => { + jasmine.clock().uninstall(); mock.restore(); resetStore(store); }); @@ -69,6 +71,11 @@ describe('Multi-file store tree actions', () => { it('adds data into tree', done => { store .dispatch('getFiles', basicCallParameters) + .then(() => { + // The populating of the tree is deferred for performance reasons. + // See this merge request for details: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25700 + jasmine.clock().tick(1); + }) .then(() => { projectTree = store.state.trees['abcproject/master'];