map positions through changes in O(N)

[pascalkuthe]

Closes #7396

Previously we iterated the entire changset for every position we mapped through the changes. That made the mapping O(MN) when mapping multiple positions and O(N^2) for most mutlicursor operations but since we are basically always dealing with sorted lists we can do the two iterations in lockstep which allows O(M+N) mapping

With this PR the example from #7396 now is pretty usable and the mapping doesn't show up in the flamegraph anymore

[dead10ck]

This is awesome! I haven't taken a close look yet, but at first glance, it unfortunately seems to squarely conflict with #7269, which relies on the fact that each range is mapped individually. 🤔 Hopefully there's an alternative I can do.

[pascalkuthe]

This is awesome! I haven't taken a close look yet, but at first glance, it unfortunately seems to squarely conflict with #7269, which relies on the fact that each range is mapped individually. 🤔 Hopefully there's an alternative I can do.

I hadn't reviewed that PR yet but I took a look and don't really think it conflicts. You are only mapping ranges trough a changeset with a single change (and do the offset tracking yourself so it's already O(N)). I deleted Range::map function because there were no uses left but if that PR still needs it then I can keep it for your used case and a documentation that it's O(N) and therefore O(MN) in loops.

This PR doesn't really remove anything. It basically just changes map_pos to operate on a sorted list instead of a single position which speedsup the most common usecase (mapping the selection trough a transaction without selsction). Mapping a single position is still possible (in fact mal_pos still exists since it's used in a few placea, its trivial to implement using once and behaves the exactly the same as on master)

[dead10ck]

Hm, yeah, I think you're right, other than Range::map getting removed, maybe nothing really changes. Even now, Selection::map isn't used in the code path of insert_char or delete_char_backwards—both still compute the resulting selection by iterating linearly over the ranges. So if I'm understanding this right, this change mostly affects the application of a change set to the document's text, rather than the computation of the transactions to begin with.

[pascalkuthe]

Yeah this only affect commands which don't manually update the transaction (also diagnostic mapping and virtual text mapping). It does however affect the "default" codepath of insert_char on mastee since that relies on the automatic range mapping (using Transaction::insert which is fairly common my used).

Our two PRs are kind of similar in that you have also included a more efficent implementation of mapping selections trough transaction by accident (by storing an offset and calling map on a single element change set). Your PR also optimizes the insert_char usecase if I read it right since you changed the entire command to use the new transaction primitive.

This PR is still useful for diagnostic/virtual text mapping and quite a few commands that still use automatic mapping. The automatic mapping is also always used for all commands for views that aren't the current view (so the selection you supply only applies to the current view). In fact the code is currently a bit unoptimized and always performs the automatic for the current view too and later just overwrites with the manually supplied selection.

[dead10ck]

This has made me realize that I think we're unnecessarily mapping the whole selection through the change set in the cases where the transaction explicitly specifies a new one.

helix/helix-view/src/document.rs

Lines 1070 to 1085 in 18160a6

	for selection in self.selections.values_mut() {
	*selection = selection
	.clone()
	// Map through changes
	.map(transaction.changes())
	// Ensure all selections across all views still adhere to invariants.
	.ensure_invariants(self.text.slice(..));
	}
	// if specified, the current selection should instead be replaced by transaction.selection
	if let Some(selection) = transaction.selection() {
	self.selections.insert(
	view_id,
	selection.clone().ensure_invariants(self.text.slice(..)),
	);
	}

We essentially do that whole mapping and then immediately throw all that work away.

[archseer]

This has made me realize that I think we're unnecessarily mapping the whole selection through the change set in the cases where the transaction explicitly specifies a new one.

The mapping still needs to happen for other views than the current one. We could speed things up slightly by removing the current view's selection from the selection set before doing the mapping if the transaction contains a selection

[pascalkuthe]

I fixed a small oversight that was causing test failures and addressed the comments. Should be ready now

[dead10ck]

🍓🫐🥞

[A-Walrus]

Just some typos :)

[the-mikedavis]

some very minor comments but otherwise this looks great, nice work!

[archseer]

Can you explain/add a comment why normalize isn't needed here? This is an additional invariant that we should document in places where no_normalize is used

[pascalkuthe]

I added a comment but this was actually exiting behaviour. Really this isn't a selection we produce here at all, we need the mapped ranges for calculating the actual selecton below (which is normalized later). In the past I achieved that by calling `map_pos' on the ranges directly (below). Normalizing here not only has no point it would also break the association between tabstops and selections (if selections are merged/sorted). I just switched to the selection function here to also take advantage of the optimization here but really this is not a selection at all we are just updating a list of ranges.

[the-mikedavis]

Github is acting strange but this is now merged onto master as d491e23 and b33516f.