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.

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

Single-pass merge for lists #13

Merged
merged 2 commits into from
Nov 30, 2015
Merged

Single-pass merge for lists #13

merged 2 commits into from
Nov 30, 2015

Conversation

vasilisp
Copy link
Contributor

This patch implements a one-pass merge operation for lists, when the patch permits. We accept patches with insert, remove, and update operations at increasing offsets The operation is now O(n + m), where n is the previous size and m is the patch size.

Diffing as per hhugo/reactiveData#12 produces patches in the above format, and the two play nicely together, but technically the two PRs can be considered independently.

We fall back to the old merging code when the patch does not meet our requirements.

This makes plain lists more tolerable, but it would still be a good idea to consider an alternative data structure (as discussed in hhugo/reactiveData#11).

@vasilisp
Copy link
Contributor Author

Any comments on this? If nobody has objections, I will merge it a day or two from now (and then rebase #12 ).

| R i :: p, _ :: l ->
linear_merge ~acc i p l
| R _ :: _, [] ->
failwith "ReactiveData.RList.linear_merge: remove from empty"
Copy link
Member

Choose a reason for hiding this comment

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

Either invalid_arg or a custom exception. Avoid failwith.

@Drup
Copy link
Member

Drup commented Oct 28, 2015

I find the invariant you use (list of patches in increasing order of indices) quite restrictive, but I suppose it's the common case in practice ? I suppose your implementation of diffing emits precisely this kind of patches ?

@Drup
Copy link
Member

Drup commented Oct 28, 2015

In any cases, I think the fast path should be documented.

@vasilisp
Copy link
Contributor Author

invalid_arg now used wherever applicable. Assertions still there for debugging purposes (no kind of input should be able to falsify them).

The invariant is OK in practice, and indeed the diffing algorithm produces such patches.

Documentation of a special case (of an internal operation) is meaningless when the general case and the whole API is not documented, so #3 :).

@vasilisp vasilisp merged commit 113f5b3 into ocsigen:master Nov 30, 2015
@vasilisp vasilisp deleted the linear-merge branch November 30, 2015 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants