add algorithm.merge

[ringabout]

Merge two sorted array:

C++ (merge algorithm):
http://www.cplusplus.com/reference/algorithm/merge

D:
https://dlang.org/library/std/algorithm/sorting/merge.html

Python(based on heap)
https://docs.python.org/3/library/heapq.html#heapq.merge

followup:

• followup on add algorithm.merge #16182 (comment) can you rename the old private algorithm.merge to algorithm.mergeAlt ?

see #17472

[ee7]

I don't have an opinion on whether this proc is worth adding, but I'd suggest that the implementation should avoid newSeqOfCap. I made a quick code suggestion that uses newSeq instead.

That gives a speedup of roughly 5x (with -d:danger --passC:-flto) when merging two sorted arrays each containing 1000 random integers in 0..10_000:

Implementation	Elapsed time
Using newSeqOfCap	11.59 ± 0.37 μs
Using newSeq	2.45 ± 0.21 μs

(And roughly 3x for two sorted arrays each containing 100 random integers in 0..1000). See this gist.

And can we avoid snake_case for identifiers? Currently this PR has:

size_x
size_y
index_x
index_y
item_x
item_y

[ringabout]

@ee7
Thanks for your help!

[ee7]

issorted -> isSorted

[ringabout]

@ee7
Thanks! (I have disabled nimsuggest because of CPU consumption :)

[ee7]

Given that this proc returns a new seq, is merged a better name?

The style guide wording suggests that we should call it merged if merge is already "available":

When both in-place mutation and 'returns transformed copy' are available the latter is a past participle of the former:

• reverse and reversed in algorithm
• sort and sorted
• rotate and rotated

algorithm.nim already has a merge proc for two var openArray, but it isn't exported, so I don't know if it's considered "available":

Nim/lib/pure/algorithm.nim

Lines 334 to 339 in 3589631

	proc merge[T](a, b: var openArray[T], lo, m, hi: int,
	cmp: proc (x, y: T): int {.closure.}, order: SortOrder) =
	# optimization: If max(left) <= min(right) there is nothing to do!
	# 1 2 3 4 ## 5 6 7 8
	# -> O(n) for sorted arrays.
	# On random data this safes up to 40% of merge calls

Also, the Nim 1.2.0 release blog post contained:

dup is a macro which turns an in-place function into one that returns a result without modifying its input. The idea is that future versions of Nim will offer only in-place versions of the functions (i.e. no more sort and sorted, rotateLeft and rotatedLeft, and so on), which could be transformed to functions returning a copy via dup.

Is that relevant here?

[ringabout]

It seems that inplace version is much slower(N * M) than the version with extra space(N + M) or complex(best performance needs quick sort, (N+M) * log(N+M)).
https://morioh.com/p/51eb8dbf58aa

[timotheecour]

the implementation should avoid newSeqOfCap

indeed. also noticed in #13448. It's so bad that newSeqOfCap should come with a performance warning in its doc comment explaining the newSeqOfCap(n) + seq.add(elem) anti-pattern. This isn't specific to nim.

[ringabout]

Should I implement inplace version(though it looks ugly to me) instead of outplace version?

[timotheecour]

Should I implement inplace version(though it looks ugly to me) instead of outplace version?

sugar.dup (or if I revive it, #13309 which has better syntax) obsoletes the need for duplicate pairs of inplace+outplace versions. This gives us N API's instead of 2N APIs.

[ringabout]

All right, I will implement the inplace version soon.

[timotheecour]

we need to decide on #16182 (comment) before merging

[timotheecour]

LGTM

[timotheecour]

• CI failure: memory allocation during {.global.} init breaks GC #17085 (comment)
• in future work, IMO we could check that (in debug mode, eg with assertions:off) that x and y are sorted, this can be done indirectly and relatively efficiently as follows: replacing result[i] = a by:

assert i == 0 or cmp(a, result[i-1]) <= 0 # or similar
result[i] = a

(can use a template since used in a few locations)

[timotheecour]

@Araq friendly ping on this