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

Infinite loop #16

Closed
randallmlough opened this issue Jan 14, 2021 · 2 comments · Fixed by hexops/valast#10
Closed

Infinite loop #16

randallmlough opened this issue Jan 14, 2021 · 2 comments · Fixed by hexops/valast#10

Comments

@randallmlough
Copy link

I've come across a possible bug where autogold gets stuck in an infinite loop, and I don't know why.

Here's a simple repo that should be reproducible on your end: https://github.com/randallmlough/autogold-infinite-loop-issue

In the ast package, uncomment the last two tests. Both those get stuck in an infinite loop.
@slimsag
Copy link
Member

slimsag commented Feb 18, 2021

Thanks for reporting this! and sorry for the delay in investigating it

I suspect it wasn't actually getting stuck in an infinite loop, but actually just taking a really long time to generate. I have recently optimized the AST formatter quite a lot on exactly these types of complex nested data types, and testing with those changes your tests complete in 4.6s now and update as expected.

Your data structure is pretty complex, though, so if you aren't happy with the 4.6s time that will probably be why - the two files clock in around 10k lines total: slimsag/autogold-infinite-loop-issue@c5415f6

In any case, v1.3.0 will be tagged here shortly and you can give that a try! Let me know if this helps :)

slimsag added a commit to hexops/valast 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 slimsag mentioned this issue Feb 18, 2021
Merged
slimsag added a commit to hexops/valast 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>
slimsag added a commit that referenced this issue Feb 18, 2021
@slimsag
update to valast@v1.4.0 (66% performance improvement, better multi-li…
bfddbc9 
…ne formatting)

* 66% performance improvement: hexops/valast#10
* Fixes performance issues on very large structures: #16
* Improves formatting of very long lines: mvdan/gofumpt#70 (comment)

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
@slimsag
Copy link
Member

slimsag commented Feb 18, 2021

Tagged, you can use the following to update:

go get -u github.com/hexops/autogold@v1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add type expression cache (66% performance improvement) hexops/valast
2 participants
@slimsag @randallmlough