Persist a subset of IdMap

[4e6]

Pull Request Description

close #9257

Changelog:

• update: Store in the file only the subset of IdMap containing the IDs used in the metadata section on the 2nd line. The full IdMap is transmitted as a parameter of the text/applyEdit request.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• The documentation has been updated, if necessary.
• All code follows the
  Scala,
  Java,
  TypeScript,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.
• Unit tests have been written where possible.

[4e6]

Please QA

[kazcw]

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

[4e6]

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.

[kazcw]

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.

[Frizi]

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.

[4e6]

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?

[Frizi]

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.

[4e6]

Updated

[JaroslavTulach]

• the direction looks good
• having some unit test would make the change more robust for the future
• what is the effect on size of IdMap of some well known example?