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.

Sign up for GitHub

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

Jump to bottom

Map key sorting could be improved #1

Open
slimsag opened this issue Dec 30, 2020 · 0 comments
Open

Map key sorting could be improved #1

slimsag opened this issue Dec 30, 2020 · 0 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@slimsag
Copy link
Member

slimsag commented Dec 30, 2020

Map keys are sorted according to regular Go rules, but a number of Go values cannot be sorted according to those rules. As it stands, valast will produce unstable output in such situations.

It would be nice to find a better way to resolve this. Feedback on how to handle this very welcome.

  • Panic or error when such a map key is encountered, ask the user provide a comparison function
  • See if go-cmp does anything for this - I suspect they don't have to because they can just compare map keys for equality
  • Take into consideration the drawbacks and benefits with the go-spew approach, output not being sorted by default, sorting by spewed keys is interesting but may have edge cases that are not handled, etc.
@slimsag slimsag added enhancement New feature or request help wanted Extra attention is needed labels Dec 30, 2020
slimsag added a commit that referenced this issue Dec 30, 2020
@slimsag
add stable sorting of map keys
fc56d3f 
Creates issue #1

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
slimsag added a commit that referenced this issue Feb 18, 2021
@slimsag
Add type expression cache (66% performance improvement)
2577a24 
With complex data types, this can provide a 66% performance improvement.

Before:

```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       2	 564812395 ns/op
PASS
ok  	github.com/hexops/valast	9.372s
```

After:

```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       6	 189646854 ns/op
PASS
ok  	github.com/hexops/valast	6.379s
```

In practice this can result in tests running much faster, before:

```
--- PASS: TestUnionMerge (6.38s)
    --- PASS: TestUnionMerge/#00 (1.42s)
    --- PASS: TestUnionMerge/#1 (1.17s)
    --- PASS: TestUnionMerge/#2 (2.32s)
    --- PASS: TestUnionMerge/#3 (0.00s)
    --- PASS: TestUnionMerge/#4 (0.00s)
    --- PASS: TestUnionMerge/#5 (1.46s)
```

After:

```
--- PASS: TestUnionMerge (2.96s)
    --- PASS: TestUnionMerge/#00 (1.05s)
    --- PASS: TestUnionMerge/#1 (0.77s)
    --- PASS: TestUnionMerge/#2 (0.77s)
    --- PASS: TestUnionMerge/#3 (0.00s)
    --- PASS: TestUnionMerge/#4 (0.00s)
    --- PASS: TestUnionMerge/#5 (0.37s)
```

Also appears to fix hexops/autogold#16

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
slimsag added a commit that referenced this issue Feb 18, 2021
@slimsag
Add type expression cache (66% performance improvement) (#10)
6fff268 
With complex data types, this can provide a 66% performance improvement.

Before:

```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       2	 564812395 ns/op
PASS
ok  	github.com/hexops/valast	9.372s
```

After:

```
$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       6	 189646854 ns/op
PASS
ok  	github.com/hexops/valast	6.379s
```

In practice this can result in tests running much faster, before:

```
--- PASS: TestUnionMerge (6.38s)
    --- PASS: TestUnionMerge/#00 (1.42s)
    --- PASS: TestUnionMerge/#1 (1.17s)
    --- PASS: TestUnionMerge/#2 (2.32s)
    --- PASS: TestUnionMerge/#3 (0.00s)
    --- PASS: TestUnionMerge/#4 (0.00s)
    --- PASS: TestUnionMerge/#5 (1.46s)
```

After:

```
--- PASS: TestUnionMerge (2.96s)
    --- PASS: TestUnionMerge/#00 (1.05s)
    --- PASS: TestUnionMerge/#1 (0.77s)
    --- PASS: TestUnionMerge/#2 (0.77s)
    --- PASS: TestUnionMerge/#3 (0.00s)
    --- PASS: TestUnionMerge/#4 (0.00s)
    --- PASS: TestUnionMerge/#5 (0.37s)
```

Also appears to fix hexops/autogold#16

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@slimsag