Skip to content

Commit

Permalink
Fix performance deterioration after many external or code-editor edit…
Browse files Browse the repository at this point in the history
…s. (#11789)

Fix performance issue reported by @JaroslavTulach on Discord.

- Fix accumulation of unreferenced AST objects when reconciling module content with `syncToCode`.
- Add a unit test checking that `syncToCode` does not allocate unneeded objects in the module.

Fixes #10768.

(cherry picked from commit 0d7427a)
  • Loading branch information
kazcw authored and jdunkerley committed Dec 6, 2024
1 parent ac5c4bf commit d56e8f7
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 24 deletions.
24 changes: 24 additions & 0 deletions app/gui/src/project-view/util/ast/__tests__/abstract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,30 @@ describe('Code edit', () => {
expect(after['func 123 arg2'].id).toBe(before['func arg1 arg2'].id)
})

test('syncToCode does not create unneeded AST nodes', () => {
const beforeRoot = Ast.parseModule('main = func 1 2\n')
beforeRoot.module.setRoot(beforeRoot)
const edit = beforeRoot.module.edit()
const newCode = 'main = func 10 2\n'
let changes: Record<string, number> | undefined = undefined
edit.observe(
(update) =>
(changes = {
added: update.nodesAdded.size,
deleted: update.nodesDeleted.size,
updated: update.nodesUpdated.size,
}),
)
edit.syncToCode(newCode)
expect(edit.root()?.code()).toBe(newCode)
expect(edit.root()?.id).toBe(beforeRoot.id)
expect(changes).toEqual({
added: 0,
deleted: 0,
updated: 1,
})
})

test('Insert argument names', () => {
const beforeRoot = Ast.parseExpression('func arg1 arg2')
assertDefined(beforeRoot)
Expand Down
13 changes: 13 additions & 0 deletions app/ydoc-shared/src/ast/mutableModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,19 @@ export class MutableModule implements Module {
return materializeMutable(this, fields) as Owned<Mutable<typeof ast>>
}

/**
* Copy the node into the module *without copying children*. The returned object, and the module containing it, may be
* in an invalid state until the callee has ensured all references are resolvable, e.g. by recursing into children.
* @internal
*/
splice<T extends Ast>(ast: T): Owned<Mutable<T>> {
assert(ast.module !== this)
const fields = ast.fields.clone() as any
this.nodes.set(ast.id, fields)
fields.set('parent', undefined)
return materializeMutable(this, fields) as Owned<Mutable<typeof ast>>
}

/** TODO: Add docs */
static Transient() {
return new this(new Y.Doc())
Expand Down
4 changes: 2 additions & 2 deletions app/ydoc-shared/src/ast/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -536,12 +536,12 @@ export function setExternalIds(edit: MutableModule, spans: SpanMap, ids: IdMap):
* context.
*/
export function parseInSameContext(
module: MutableModule,
code: string,
ast: Ast,
module?: MutableModule,
): { root: Owned; spans: SpanMap } {
const rawParsed = rawParseInContext(code, getParseContext(ast))
return abstract(module, rawParsed, code)
return abstract(module ?? MutableModule.Transient(), rawParsed, code)
}

type ParseContext = 'module' | 'block' | 'expression' | 'statement'
Expand Down
45 changes: 23 additions & 22 deletions app/ydoc-shared/src/ast/syncToCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
type AstId,
MutableAssignment,
type MutableAst,
type Owned,
rewriteRefs,
syncFields,
syncNodeMetadata,
Expand Down Expand Up @@ -175,38 +174,36 @@ export function applyTextEditsToAst(
) {
const printed = printWithSpans(ast)
const code = applyTextEdits(printed.code, textEdits)
ast.module.transact(() => {
const parsed = parseInSameContext(ast.module, code, ast)
const toSync = calculateCorrespondence(
ast,
printed.info.nodes,
parsed.root,
parsed.spans.nodes,
textEdits,
code,
)
syncTree(ast, parsed.root, toSync, ast.module, metadataSource)
})
const parsed = parseInSameContext(code, ast)
const toSync = calculateCorrespondence(
ast,
printed.info.nodes,
parsed.root,
parsed.spans.nodes,
textEdits,
code,
)
ast.module.transact(() => syncTree(ast, parsed.root, toSync, ast.module, metadataSource))
}

/** Replace `target` with `newContent`, reusing nodes according to the correspondence in `toSync`. */
function syncTree(
target: MutableAst,
newContent: Owned,
newContent: Ast,
toSync: Map<AstId, Ast>,
edit: MutableModule,
metadataSource: Module,
) {
const newIdToEquivalent = new Map<AstId, AstId>()
for (const [beforeId, after] of toSync) newIdToEquivalent.set(after.id, beforeId)
const childReplacerFor = (parentId: AstId) => (id: AstId) => {
const childSplicerFor = (parentId: AstId) => (id: AstId) => {
const original = newIdToEquivalent.get(id)
if (original) {
const replacement = edit.get(original)
if (replacement.parentId !== parentId) replacement.fields.set('parent', parentId)
return original
} else {
const child = edit.get(id)
const child = edit.splice(newContent.module.get(id)!)
if (child.parentId !== parentId) child.fields.set('parent', parentId)
}
}
Expand All @@ -215,12 +212,16 @@ function syncTree(
const parent = edit.get(parentId)
const targetSyncEquivalent = toSync.get(target.id)
const syncRoot = targetSyncEquivalent?.id === newContent.id ? targetSyncEquivalent : undefined
if (!syncRoot) {
parent.replaceChild(target.id, newContent)
newContent.fields.set('metadata', target.fields.get('metadata').clone())
let newRoot: MutableAst
if (syncRoot) {
newRoot = target
} else {
const spliced = edit.splice(newContent)
parent.replaceChild(target.id, spliced)
newRoot = spliced
newRoot.fields.set('metadata', target.fields.get('metadata').clone())
target.fields.get('metadata').set('externalId', newExternalId())
}
const newRoot = syncRoot ? target : newContent
newRoot.visitRecursive(ast => {
const syncFieldsFrom = toSync.get(ast.id)
const editAst = edit.getVersion(ast)
Expand All @@ -229,7 +230,7 @@ function syncTree(
ast instanceof Assignment ?
metadataSource.get(ast.fields.get('expression').node)
: undefined
syncFields(edit.getVersion(ast), syncFieldsFrom, childReplacerFor(ast.id))
syncFields(editAst, syncFieldsFrom, childSplicerFor(ast.id))
if (editAst instanceof MutableAssignment && originalAssignmentExpression) {
if (editAst.expression.externalId !== originalAssignmentExpression.externalId)
editAst.expression.setExternalId(originalAssignmentExpression.externalId)
Expand All @@ -239,7 +240,7 @@ function syncTree(
)
}
} else {
rewriteRefs(editAst, childReplacerFor(ast.id))
rewriteRefs(editAst, childSplicerFor(ast.id))
}
return true
})
Expand Down

0 comments on commit d56e8f7

Please sign in to comment.