improvement(query): performance improvement for sorted merge iterator

[foobar]

Sorted merge iterator has cpu-intensive operations to sort the points
from multiple inputs. Typical queries like SELECT * FROM m GROUP BY *
do not behave well due to the comparison of points though in many cases
it doesn't necessarily have to use the slow path.

This patch adds a shortcut. If each input has a single and unique
series we can just return the points input by input.
The detection of the shortcut introduces slight overhead but the gains
are significant in many slow queries.

See #8304

[foobar]

@jsternberg @jacobmarble
could you take a look?

[e-dard]

Hi @foobar thanks for this PR. Very cool!

There are a few things of note:

1. I have a couple of small suggestions. The main one is to remove the use of +1, and just use 1 instead, but I also made a simplification to the code and a possible short circuit.
2. I don't know if we will put this work into a 1.7 release. Can you change the base branch to master-1.x? If accepted, we can manage the backport to the 1.8 branch.
3. I would like to see some benchmarks showing the changes in performance for this change. I mainly concerned about whether the extra work to detect if the fast condition will be used will be significantly detrimental to other use-cases (queries).
4. can you add some test coverage for detectFast in the form of unit tests?

@stuartcarnie would you please review this too.

Thanks again @foobar for the contribution!

[e-dard]

@foobar hi, I'm happy with what's here so far. However, I still would like to see some benchmarking and testing of the new code-path. Further @stuartcarnie may have his own suggestions. Thanks

[foobar]

Thanks for your comments! @e-dard

1. I have a couple of small suggestions. The main one is to remove the use of +1, and just use 1 instead, but I also made a simplification to the code and a possible short circuit.

I reworked the code based on your suggestion.

2. I don't know if we will put this work into a 1.7 release. Can you change the base branch to master-1.x? If accepted, we can manage the backport to the 1.8 branch.

done

3. I would like to see some benchmarks showing the changes in performance for this change. I mainly concerned about whether the extra work to detect if the fast condition will be used will be significantly detrimental to other use-cases (queries).

benchmark test added and here is the result on my machine:

go test -bench=BenchmarkSorted ./query/...
goos: linux
goarch: amd64
pkg: github.com/influxdata/influxdb/query
BenchmarkSortedMergeIterator_Fast-32                         166           6738013 ns/op
BenchmarkSortedMergeIterator_NotFast-32                       19          61152186 ns/op
BenchmarkSortedMergeIterator_FastCheckOverhead-32        1718110               700 ns/op

4. can you add some test coverage for detectFast in the form of unit tests?

added test cases

[stuartcarnie]

Overall, this is a great improvement 🥇

May I suggest the following small change, which may be a little easier for a future maintainer to understand. It is also likely to be a little more efficient and avoids the use of strings.Compare, which is not recommended by the Go documentation and the source itself:

	var less func(i, j int) bool
	if h.opt.Ascending {
		less = func(i, j int) bool {
			x, y := s[i].point, s[j].point
			if x.Name != y.Name {
				return x.Name < y.Name
			}

			if x.Tags.ID() != y.Tags.ID() {
				return x.Tags.ID() < y.Tags.ID()
			}

			hasDup = true
			return false
		}
	} else {
		less = func(i, j int) bool {
			x, y := s[i].point, s[j].point
			if x.Name != y.Name {
				return x.Name > y.Name
			}

			if x.Tags.ID() != y.Tags.ID() {
				return x.Tags.ID() > y.Tags.ID()
			}

			hasDup = true
			return false
		}
	}
	sort.Slice(s, less)

[foobar]

Thanks @stuartcarnie for reviewing this PR.

May I suggest the following small change, which may be a little easier for a future maintainer to understand. It is also likely to be a little more efficient and avoids the use of strings.Compare, which is not recommended by the Go documentation and [the source itself]

From the source it shouldn't have significant difference in term of performance; for readability, current code is shorter with @e-dard comments. I'm fine with either.
@e-dard your thought?

[foobar]

@e-dard @stuartcarnie @rickspencer3 any other comments?

[e-dard]

@stuartcarnie can you run with this? I got a bunch of other review backed up :-)

[stuartcarnie]

I am ok, as-is. Thanks again, @foobar!

@dgnorton if you are happy, you are welcome to merge it

[foobar]

hi @dgnorton, had you got a chance to look at this ?

[stuartcarnie]

@dgnorton everything looks good to me

[foobar]

@dgnorton could you get it merged?

[ayang64]

@foobar, @stuartcarnie

Please forgive me if I'm off base -- I haven't tested this thoroughly but I think the entire less func could be simplified to something like this:

less := func(i, j int) bool {
  x, y := s[i].point, s[j].point
  hasDup = hasDup ||  x.Name == y.Name
  return ((x.Name < y.Name) || (x.Tags.ID() < y.Tags.ID()) && h.opt.Ascending
}

this should be a bit faster. i haven't tested or benchmarked it though.

[ayang64]

please investigate if something like the following would work as a comparator:

less := func(i, j int) bool {
  x, y := s[i].point, s[j].point
  hasDup = hasDup || x.Name == y.Name
  return ((x.Name < y.Name) || (x.Tags.ID() < y.Tags.ID()) && h.opt.Ascending
}

i think that should be at least as fast.

[stuartcarnie]

@ayang64 the less function is required to sort ascending or descending order depending on h.opt.Ascending. It is unclear to me if the short version above achieves that. Also, your proposed version is setting hasDup to true only if x.Name == y.Name, whereas the original function sets hasDup to true iif both the Name and Tags are equal, which is a required property.

I didn't see any obvious performance issues with the existing code, and the overall improvement is significant.

[foobar]

hi @timhallinflux, could this catch 1.8.1 milestone?

See Stuarts response