Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist a subset of IdMap #10347

Merged
merged 7 commits into from
Jul 8, 2024
Merged

Persist a subset of IdMap #10347

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4bd7eb7
DRAFT: idmap diff
4e6 Jun 22, 2024
86542fe
DRAFT: fix opening issue
4e6 Jun 24, 2024
3e124c0
misc: cleanup
4e6 Jun 24, 2024
21fd249
Merge branch 'develop' into wip/db/9257-remove-idmap-from-source-file
4e6 Jun 25, 2024
9e864b6
Merge branch 'develop' into wip/db/9257-remove-idmap-from-source-file
4e6 Jul 8, 2024
a7d81fe
misc: review comments
4e6 Jul 8, 2024
d276905
misc: review comments
4e6 Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/gui2/shared/languageServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import type {
ExpressionId,
FileEdit,
FileSystemObject,
IdMapTriple,
IdMapTuple,
Notifications,
Path,
RegisterOptions,
Expand Down Expand Up @@ -270,8 +272,12 @@ export class LanguageServer extends ObservableV2<Notifications & TransportEvents
}

/** [Documentation](https://github.com/enso-org/enso/blob/develop/docs/language-server/protocol-language-server.md#textapplyedit) */
applyEdit(edit: FileEdit, execute: boolean): Promise<LsRpcResult<void>> {
return this.request('text/applyEdit', { edit, execute })
applyEdit(
edit: FileEdit,
execute: boolean,
idMap?: IdMapTriple[] | IdMapTuple[],
): Promise<LsRpcResult<void>> {
return this.request('text/applyEdit', { edit, execute, idMap })
}

/** [Documentation](https://github.com/enso-org/enso/blob/develop/docs/language-server/protocol-language-server.md#filewrite) */
Expand Down
9 changes: 9 additions & 0 deletions app/gui2/shared/languageServerTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ export interface Position {
character: number
}

interface IdMapSpan {
index: { value: number }
size: { value: number }
}

export type IdMapTuple = [IdMapSpan, string]

export type IdMapTriple = [number, number, string]
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved

export type RegisterOptions = { path: Path } | { contextId: ContextId } | {}

export interface CapabilityRegistration {
Expand Down
29 changes: 21 additions & 8 deletions app/gui2/ydoc-server/languageServerSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
translateVisualizationFromFile,
} from './edits'
import * as fileFormat from './fileFormat'
import { deserializeIdMap, serializeIdMap } from './serialization'
import { deserializeIdMap, idMapToArray, serializeIdMap } from './serialization'
import { WSSharedDoc } from './ydoc'

const SOURCE_DIR = 'src'
Expand Down Expand Up @@ -457,6 +457,11 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
}
}

private static getIdMapToPersist(idMap: IdMap, metadata: fileFormat.IdeMetadata['node']): IdMap {
const entriesIntersection = idMap.entries().filter(([, id]) => id in metadata)
return new IdMap(entriesIntersection)
}

private sendLsUpdate(
synced: EnsoFileParts,
newCode: string | undefined,
Expand All @@ -466,13 +471,21 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
if (this.syncedContent == null || this.syncedVersion == null) return

const code = newCode ?? synced.code
const newMetadataJson =
newMetadata &&
json.stringify({ ...this.syncedMeta, ide: { ...this.syncedMeta.ide, node: newMetadata } })
const newIdMapJson = newIdMap && serializeIdMap(newIdMap)
const metadataToPersist = {
...this.syncedMeta,
ide: { ...this.syncedMeta.ide, node: newMetadata },
}
const newMetadataJson = newMetadata && json.stringify(metadataToPersist)
4e6 marked this conversation as resolved.
Show resolved Hide resolved
const idMapToPersist =
newIdMap &&
Copy link
Contributor

@kazcw kazcw Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is no longer accurate: A new idMapToPersist needs to be computed if the ID map changes or if the metadata changes.

Copy link
Contributor Author

@4e6 4e6 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the metadata line changes, the IdMap will be recomputed as well. And if the metadata was changed without changing the IDs, we don't need to edit the IdMap section.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the metadata line changes, the IdMap will be recomputed as well.

I don't see this being enforced in the ydoc-server. If you mean it will be recomputed at the same time in the GUI, the ydoc-server should not depend on that.

When adding a new node, first the new node is committed then its position is computed. In the interim, the node usually doesn't have any non-default metadata; if we are always writing node metadata at the same time that a node is added, that's an inefficiency we should be able to fix in the GUI without the ydoc-server causing data loss.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a problem. There needs to be a check if newMetadata contains a new key. At the minimum, This could be (newIdMap || newMetadata) &&, effectively always writing ID Map when any metadata changes. That could be further optimized later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that applyDocumentUpdates

enso/app/gui2/ydoc-server/languageServerSession.ts

Lines 451 to 455 in 305f69c

const { newCode, newIdMap, newMetadata } = applyDocumentUpdates(
this.doc,
synced,
moduleUpdate,
)

recomputes IdMap from the root AST

enso/app/gui2/ydoc-server/edits.ts

Lines 67 to 74 in 305f69c

const syncModule = new MutableModule(doc.ydoc)
const root = syncModule.root()
assert(root != null)
if (codeChanged || idsChanged || synced.idMapJson == null) {
const { code, info } = print(root)
if (codeChanged) newCode = code
newIdMap = spanMapToIdMap(info)
}

or am I reading that wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition codeChanged || idsChanged || synced.idMapJson == null can still be false when existing metadata structure updates. In that situation a new ID will have to be stored while no code was actually changed.

Currently, all metadata is written on expressions that also happen to be node roots, so it kinda works out. But this is not a limitation of the system that we are willing to have, since we are soon going to add metadata to individual expressions. That will be needed to store persistent widget state. Those expressions will already exist when we adjust the widget data, effectively sending a delaying metadata update without any code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

ModulePersistence.getIdMapToPersist(
newIdMap,
metadataToPersist.ide.node ?? this.syncedMeta.ide.node,
4e6 marked this conversation as resolved.
Show resolved Hide resolved
)
const newIdMapToPersistJson = idMapToPersist && serializeIdMap(idMapToPersist)
const newContent = combineFileParts({
code,
idMapJson: newIdMapJson ?? synced.idMapJson ?? '[]',
idMapJson: newIdMapToPersistJson ?? synced.idMapJson ?? '[]',
metadataJson: newMetadataJson ?? synced.metadataJson ?? '{}',
})

Expand Down Expand Up @@ -502,7 +515,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {

const execute = newCode != null || newIdMap != null
const edit: FileEdit = { path: this.path, edits, oldVersion: this.syncedVersion, newVersion }
const apply = this.ls.applyEdit(edit, execute)
const apply = this.ls.applyEdit(edit, execute, newIdMap && idMapToArray(newIdMap))
const handleError = (error: unknown) => {
console.error('Could not apply edit:', error)
// Try to recover by reloading the file.
Expand All @@ -521,7 +534,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
this.syncedVersion = newVersion
if (newMetadata) this.syncedMeta.ide.node = newMetadata
if (newCode) this.syncedCode = newCode
if (newIdMapJson) this.syncedIdMap = newIdMapJson
if (newIdMapToPersistJson) this.syncedIdMap = newIdMapToPersistJson
if (newMetadataJson) this.syncedMetaJson = newMetadataJson
this.setState(LsSyncState.Synchronized)
}, handleError)
Expand Down
2 changes: 1 addition & 1 deletion app/gui2/ydoc-server/serialization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function serializeIdMap(map: IdMap): string {
return json.stringify(idMapToArray(map))
}

function idMapToArray(map: IdMap): fileFormat.IdMapEntry[] {
export function idMapToArray(map: IdMap): fileFormat.IdMapEntry[] {
const entries: fileFormat.IdMapEntry[] = []
map.entries().forEach(([rangeBuffer, id]) => {
const decoded = sourceRangeFromKey(rangeBuffer)
Expand Down
Loading