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

String formatting could be improved #2

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

String formatting could be improved #2

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

Comments

@emidoots
Copy link
Member

Today valast uses a dumb heuristic to determine if a string should be formatting as a Go "string literal" or `raw string literal`:

		if len(s) > 40 && strings.Contains(s, "\n") && !strings.Contains(s, "`") {
			return basicLit(vv, token.STRING, "string", "`"+s+"`", opt)
		}
		return basicLit(vv, token.STRING, "string", strconv.Quote(v.String()), opt)

No doubt there are cases where the formatting provided here will be less than optimal. Problematic cases include:

  • Very long single-line strings.
  • Multi-line strings with long lines.
  • Strings with lots of unicode where escape sequences (or not) may be more desirable.
  • ...

The goal of this issue is to find out how we can improve the default formatting to meet most use cases. It would then additionally be nice to have the ability for users of the package to provide a string for matter of their own, but we should do this after exhausting possibilities of improving the default.
@emidoots emidoots added enhancement New feature or request help wanted Extra attention is needed labels Dec 30, 2020
emidoots pushed a commit that referenced this issue Dec 30, 2020
improve default string formatting
c45fd0a 
Creates issue #2

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
emidoots pushed a commit that referenced this issue Feb 12, 2021
add VALAST_PROFILE to emit performance profiling information
ac3cf79 
This can be useful for debugging why some complex/nested data structures, such
as those reported in https://github.com/sourcegraph/sourcegraph/pull/18189
are not being handled quickly by Valast:

```
$ VALAST_PROFILE=t go test -v -run='TestUnionMerge/#2' ./cmd/frontend/graphqlbackend/
=== RUN   TestUnionMerge
=== RUN   TestUnionMerge/#2
valast: profile
1242026902ns: *graphqlbackend.SearchResultsResolver
  1242001613ns: graphqlbackend.SearchResultsResolver
    1241965682ns: []graphqlbackend.SearchResultResolver
      631554070ns: *graphqlbackend.RepositoryResolver
        631552406ns: *graphqlbackend.RepositoryResolver
          631549333ns: graphqlbackend.RepositoryResolver
            631541623ns: *types.Repo
              409622256ns: types.Repo
                194531303ns: api.RepoName
      610281838ns: *graphqlbackend.RepositoryResolver
        610280747ns: *graphqlbackend.RepositoryResolver
          610277705ns: graphqlbackend.RepositoryResolver
            610242182ns: *types.Repo
              408077957ns: types.Repo
                185258694ns: api.RepoName
      7893ns: *graphqlbackend.FileMatchResolver
        7140ns: *graphqlbackend.FileMatchResolver
          5401ns: graphqlbackend.FileMatchResolver
            3438ns: graphqlbackend.FileMatch
              536ns: string
      33541ns: *graphqlbackend.FileMatchResolver
        31856ns: *graphqlbackend.FileMatchResolver
          29326ns: graphqlbackend.FileMatchResolver
            15457ns: graphqlbackend.FileMatch
              581ns: string
      7347ns: *graphqlbackend.CommitSearchResultResolver
        6671ns: *graphqlbackend.CommitSearchResultResolver
          5591ns: graphqlbackend.CommitSearchResultResolver
            509ns: string
            1856ns: *graphqlbackend.highlightedString
              702ns: graphqlbackend.highlightedString
      14634ns: *graphqlbackend.CommitSearchResultResolver
        13880ns: *graphqlbackend.CommitSearchResultResolver
          12711ns: graphqlbackend.CommitSearchResultResolver
            623ns: string
            8674ns: *graphqlbackend.highlightedString
              7160ns: graphqlbackend.highlightedString
      7210ns: *graphqlbackend.CommitSearchResultResolver
        6198ns: *graphqlbackend.CommitSearchResultResolver
          4549ns: graphqlbackend.CommitSearchResultResolver
            591ns: string
      49168ns: *graphqlbackend.CommitSearchResultResolver
        34915ns: *graphqlbackend.CommitSearchResultResolver
          27193ns: graphqlbackend.CommitSearchResultResolver
valast: profile
1246689469ns: *graphqlbackend.SearchResultsResolver
  1246683327ns: graphqlbackend.SearchResultsResolver
    1246675918ns: []graphqlbackend.SearchResultResolver
      605954511ns: *graphqlbackend.RepositoryResolver
        605951599ns: *graphqlbackend.RepositoryResolver
          605948656ns: graphqlbackend.RepositoryResolver
            605939227ns: *types.Repo
              398690970ns: types.Repo
                196774754ns: api.RepoName
      640656293ns: *graphqlbackend.RepositoryResolver
        640655289ns: *graphqlbackend.RepositoryResolver
          640647478ns: graphqlbackend.RepositoryResolver
            640634530ns: *types.Repo
              415481187ns: types.Repo
                191840428ns: api.RepoName
      11750ns: *graphqlbackend.FileMatchResolver
        11086ns: *graphqlbackend.FileMatchResolver
          9437ns: graphqlbackend.FileMatchResolver
            7961ns: graphqlbackend.FileMatch
              657ns: string
      7593ns: *graphqlbackend.FileMatchResolver
        6844ns: *graphqlbackend.FileMatchResolver
          5599ns: graphqlbackend.FileMatchResolver
            3269ns: graphqlbackend.FileMatch
              497ns: string
      10136ns: *graphqlbackend.CommitSearchResultResolver
        9470ns: *graphqlbackend.CommitSearchResultResolver
          8263ns: graphqlbackend.CommitSearchResultResolver
            484ns: string
            1711ns: *graphqlbackend.highlightedString
              530ns: graphqlbackend.highlightedString
      7516ns: *graphqlbackend.CommitSearchResultResolver
        6869ns: *graphqlbackend.CommitSearchResultResolver
          5810ns: graphqlbackend.CommitSearchResultResolver
            585ns: string
            2194ns: *graphqlbackend.highlightedString
              706ns: graphqlbackend.highlightedString
      5283ns: *graphqlbackend.CommitSearchResultResolver
        4576ns: *graphqlbackend.CommitSearchResultResolver
          3292ns: graphqlbackend.CommitSearchResultResolver
            544ns: string
      9954ns: *graphqlbackend.CommitSearchResultResolver
        8253ns: *graphqlbackend.CommitSearchResultResolver
          6366ns: graphqlbackend.CommitSearchResultResolver
--- PASS: TestUnionMerge (2.78s)
    --- PASS: TestUnionMerge/#2 (2.78s)
PASS
ok  	github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend	3.230s
```

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
emidoots pushed a commit that referenced this issue Feb 12, 2021
add VALAST_PROFILE to emit performance profiling information (#6)
2efbde8 
This can be useful for debugging why some complex/nested data structures, such
as those reported in https://github.com/sourcegraph/sourcegraph/pull/18189
are not being handled quickly by Valast:

```
$ VALAST_PROFILE=t go test -v -run='TestUnionMerge/#2' ./cmd/frontend/graphqlbackend/
=== RUN   TestUnionMerge
=== RUN   TestUnionMerge/#2
valast: profile
1242026902ns: *graphqlbackend.SearchResultsResolver
  1242001613ns: graphqlbackend.SearchResultsResolver
    1241965682ns: []graphqlbackend.SearchResultResolver
      631554070ns: *graphqlbackend.RepositoryResolver
        631552406ns: *graphqlbackend.RepositoryResolver
          631549333ns: graphqlbackend.RepositoryResolver
            631541623ns: *types.Repo
              409622256ns: types.Repo
                194531303ns: api.RepoName
      610281838ns: *graphqlbackend.RepositoryResolver
        610280747ns: *graphqlbackend.RepositoryResolver
          610277705ns: graphqlbackend.RepositoryResolver
            610242182ns: *types.Repo
              408077957ns: types.Repo
                185258694ns: api.RepoName
      7893ns: *graphqlbackend.FileMatchResolver
        7140ns: *graphqlbackend.FileMatchResolver
          5401ns: graphqlbackend.FileMatchResolver
            3438ns: graphqlbackend.FileMatch
              536ns: string
      33541ns: *graphqlbackend.FileMatchResolver
        31856ns: *graphqlbackend.FileMatchResolver
          29326ns: graphqlbackend.FileMatchResolver
            15457ns: graphqlbackend.FileMatch
              581ns: string
      7347ns: *graphqlbackend.CommitSearchResultResolver
        6671ns: *graphqlbackend.CommitSearchResultResolver
          5591ns: graphqlbackend.CommitSearchResultResolver
            509ns: string
            1856ns: *graphqlbackend.highlightedString
              702ns: graphqlbackend.highlightedString
      14634ns: *graphqlbackend.CommitSearchResultResolver
        13880ns: *graphqlbackend.CommitSearchResultResolver
          12711ns: graphqlbackend.CommitSearchResultResolver
            623ns: string
            8674ns: *graphqlbackend.highlightedString
              7160ns: graphqlbackend.highlightedString
      7210ns: *graphqlbackend.CommitSearchResultResolver
        6198ns: *graphqlbackend.CommitSearchResultResolver
          4549ns: graphqlbackend.CommitSearchResultResolver
            591ns: string
      49168ns: *graphqlbackend.CommitSearchResultResolver
        34915ns: *graphqlbackend.CommitSearchResultResolver
          27193ns: graphqlbackend.CommitSearchResultResolver
valast: profile
1246689469ns: *graphqlbackend.SearchResultsResolver
  1246683327ns: graphqlbackend.SearchResultsResolver
    1246675918ns: []graphqlbackend.SearchResultResolver
      605954511ns: *graphqlbackend.RepositoryResolver
        605951599ns: *graphqlbackend.RepositoryResolver
          605948656ns: graphqlbackend.RepositoryResolver
            605939227ns: *types.Repo
              398690970ns: types.Repo
                196774754ns: api.RepoName
      640656293ns: *graphqlbackend.RepositoryResolver
        640655289ns: *graphqlbackend.RepositoryResolver
          640647478ns: graphqlbackend.RepositoryResolver
            640634530ns: *types.Repo
              415481187ns: types.Repo
                191840428ns: api.RepoName
      11750ns: *graphqlbackend.FileMatchResolver
        11086ns: *graphqlbackend.FileMatchResolver
          9437ns: graphqlbackend.FileMatchResolver
            7961ns: graphqlbackend.FileMatch
              657ns: string
      7593ns: *graphqlbackend.FileMatchResolver
        6844ns: *graphqlbackend.FileMatchResolver
          5599ns: graphqlbackend.FileMatchResolver
            3269ns: graphqlbackend.FileMatch
              497ns: string
      10136ns: *graphqlbackend.CommitSearchResultResolver
        9470ns: *graphqlbackend.CommitSearchResultResolver
          8263ns: graphqlbackend.CommitSearchResultResolver
            484ns: string
            1711ns: *graphqlbackend.highlightedString
              530ns: graphqlbackend.highlightedString
      7516ns: *graphqlbackend.CommitSearchResultResolver
        6869ns: *graphqlbackend.CommitSearchResultResolver
          5810ns: graphqlbackend.CommitSearchResultResolver
            585ns: string
            2194ns: *graphqlbackend.highlightedString
              706ns: graphqlbackend.highlightedString
      5283ns: *graphqlbackend.CommitSearchResultResolver
        4576ns: *graphqlbackend.CommitSearchResultResolver
          3292ns: graphqlbackend.CommitSearchResultResolver
            544ns: string
      9954ns: *graphqlbackend.CommitSearchResultResolver
        8253ns: *graphqlbackend.CommitSearchResultResolver
          6366ns: graphqlbackend.CommitSearchResultResolver
--- PASS: TestUnionMerge (2.78s)
    --- PASS: TestUnionMerge/#2 (2.78s)
PASS
ok  	github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend	3.230s
```

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
emidoots pushed a commit that referenced this issue Feb 18, 2021
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>
emidoots pushed a commit that referenced this issue Feb 18, 2021
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
@emidoots