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

sort: improve implementation #45422

Closed
aimuz opened this issue Apr 7, 2021 · 3 comments
Closed

sort: improve implementation #45422

aimuz opened this issue Apr 7, 2021 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@aimuz
Copy link
Contributor

aimuz commented Apr 7, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

Can slice sorting be re implemented? After reading the code, I found that slice sorting relies on a genzfunction. The function of this program is to copy sort a piece of code and make some minor modifications. I wonder if I can extract a common interface to simplify this part of logic and remove the genzfunction dependency.

// Copyright 2009 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

//go:generate go run genzfunc.go

// Package sort provides primitives for sorting slices and user-defined collections.
package sort

I think it's possible to extract a lessswap interface? To achieve the same purpose, for example

type Interface interface {
	// Len is the number of elements in the collection.
	Len() int

	lessSwap
}

type lessSwap interface {
	Less(i, j int) bool
	Swap(i, j int)
}
type sliceLessSwap struct {
	less func(i, j int) bool
	swap func(i, j int)
}

func (s sliceLessSwap) Less(i, j int) bool {
	return s.less(i, j)
}

func (s sliceLessSwap) Swap(i, j int) {
	s.swap(i, j)
}

quickSort Change the parameter to the following type

func quickSort(data lessSwap, a, b, maxDepth int)
func Slice(x interface{}, less func(i, j int) bool) {
	rv := reflectValueOf(x)
	swap := reflectSwapper(x)
	length := rv.Len()
	quickSort(sliceLessSwap{less, swap}, 0, length, maxDepth(length))
}

What do you think of this proposal? I think this can reduce the complexity of maintenance

What did you expect to see?

What did you see instead?

@davecheney
Copy link
Contributor

Thank you for raising this issue. I encourage you to use the benchmarks build into the sort package and if there is a measurable improvement consider contributing the change, https://golang.org/doc/contribute

@seankhliao seankhliao changed the title Re implementation of sort slice sort: improve implementation Apr 7, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 7, 2021
@ericlagergren
Copy link
Contributor

See this CL for an example of why this might not be a good idea: https://go-review.googlesource.com/c/go/+/27456

@ianlancetaylor
Copy link
Contributor

Sorting is a particular and unusual case where speed is more important than code readability, within reason. If you can make the sort code more readable without sacrificing speed, that would be great. We would welcome such a contribution. But note that the code is written the way that it is specifically so that it runs faster.

I'm going to close this issue because I don't think there is any action for us to take. Please comment if you disagree.

@golang golang locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants