[Go] Improve BitSet implementation

[jcking]

The current BitSet implementation uses a map. The ideal implementation uses a slice of integers and has significantly less overhead. This pull request switches BitSet to be backed by a slice of integers. This is very similar to how things like std::bitset in C++ are implemented.

[jcking]

@parrt Not sure who deals with Go.

[parrt]

I think @pboyer, original dev, isn't able to help anymore, but @KvanTTT has been working on perf also.

[parrt]

Getting a speed check and then will check this one. Go target is really slow at moment and this is likely culprit.

[KvanTTT]

@jcking is it possible to look at a comparison table before/after optimization? Have you checked it?

[parrt]

real 4m14.464s on my m1 mac mini and now I'm running with these tests in.
real 4m23.657s so a little bit longer or about the same. Still a map is going to be much less efficient than a bit set, at least in terms of memory. Maybe @KvanTTT can take a look.

[KvanTTT]

Have you tested performance on runtime tests? It is not reliable because of a lot of side effects and IO operation. The result may vary significantly. You can try to do it several times or take a look at two sequential AppVeyour builts: 1, 2. There is a difference in 95 sec without Go code change at all.

[parrt]

ah. yeah, good point. Not thinking straight today. Maybe you can test using the rig you used for your patches?

[KvanTTT]

I have not used any rig because it was obvious and significant changes (22 -> 6 min for javascript, 20 -> 8 min for Go). But this request looks like micro-optimization. I have no objection to merging but ideally, it also should be tested using micro-benchmarks (as any merging code).

[jcking]

I can get the Go tests to run in 1 min 50 sec instead of 3 min 44 sec by making them parallel. It looks like the Go tests copy the contents of GOROOT. It might be better to just override GOPATH and avoid the copy.

[jcking]

Its also not really a micro-optimization. If you run in a for loop and set bits 0 through 63 on the map impl, its going to consume more memory and be less efficient then the slice implementation. The slice implementation will only allocate once, while the map one will have to resize and rehash at least once. It will also have to perform more allocations, depending on map underlying implementation. Using the slice reduces the amount of generate garbage for GC. A micro-optimization would be to take that further and avoid the slice for single element bitsets and use just a bare uint64.

[parrt]

@jcking maybe a good excuse for me to learn some more Go. wow. copying entire root? strange. ok, i'll look. maybe it was old-school Go before modules.

@KvanTTT i'll try to run a Go parser...

I'll finish #3407 first though.

[KvanTTT]

I can get the Go tests to run in 1 min 50 sec instead of 3 min 44 sec by making them parallel. It looks like the Go tests copy the contents of GOROOT.

It's true. But I was not able to override GOPATH because of some problems, tests were not working. Anyway, single copying of the content of GOPATH is better than the previous scheme with copying ALL runtime files for EVERY test.

Take a look at this pull request, I tried GOPATH redirection: #3365 But maybe you'll be able to fix it.

[KvanTTT]

I remembered it's not just copying, it's copying together with ANTLR runtime files. Redirection occurs but to the new directory that contains both go system files and go ANTLR runtime files: https://github.com/antlr/antlr4/blob/master/runtime-testsuite/test/org/antlr/v4/test/runtime/go/BaseGoTest.java#L182 I don't know how to implement it in a better way. Maybe copy go system files only one time and cache it for further using but go version also can be updated.

[parrt]

Yeah, maybe I shouldn't mess with the Go test rig until I know how it deals with modules and installing software better.

[KvanTTT]

I don't think so because grammars-v4 uses a stable version from the maven repository, this change is in the development branch.

[KvanTTT]

Maybe I'm wrong. Is it possible to set up downloading from a certain commit?

[KvanTTT]

It looks like it's a good idea to run tests on grammar-v4 together with ordinary runtime tests.

[jcking]

#3427 should resolve the issue. I missed that specific case.

…

On Wed, Dec 22, 2021 at 3:08 AM Ivan Kochurkin ***@***.***> wrote: It looks like it's a good idea to run tests on grammar-v4 together with ordinary runtime tests. — Reply to this email directly, view it on GitHub <#3421 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHTURXKL65Q6LCE55LPOILUSGWTRANCNFSM5KOHZERQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[parrt]

@KvanTTT @jcking

Hi. just did some testing. I built first with

a4.9.3 -Dlanguage=Go -o parser *.g4

(where a4.9.3 is an alias to run the tool.)

Not sure I'm actually using the go.mod file correctly, but here is what I have:

module go-parse-performance

go 1.17

require github.com/antlr/antlr4/runtime/Go/antlr v0.0.0-20220102222715-89f7760263f1 // indirect

replace github.com/antlr/antlr4/runtime/Go/antlr => /Users/parrt/tmp/antlr4.9.3/runtime/Go/antlr

That yields (subset of files in jdk 1.8) timing:

Parsed 501 files

real 0m13.971s
user 0m24.560s
sys 0m1.007s

Then `go.mod`:

module go-parse-performance

go 1.17

require github.com/antlr/antlr4/runtime/Go/antlr v0.0.0-20220102222715-89f7760263f1 // indirect

replace github.com/antlr/antlr4/runtime/Go/antlr => /Users/parrt/antlr/code/antlr4/runtime/Go/antlr

Gives:

Parsed 501 files (4.10 on disk I hope)

real 0m14.186s
user 0m23.983s
sys 0m1.123s

So, about the same.  Note it bounces around a bit depending on sys load of course.

```go
package main

import (
	"fmt"
	"github.com/antlr/antlr4/runtime/Go/antlr"
	"go-parse-performance/parser"
	"io/fs"
	"io/ioutil"
	"path/filepath"
	"strings"
)

func main() {
	// /Library/Java/JavaVirtualMachines/jdk-11.0.13.jdk/Contents/Home/lib/src.zip
	// /Library/Java/JavaVirtualMachines/jdk1.8.0_311.jdk/Contents/Home/src.zip
	// I unzip one of those into a local subdirectory
	root := "jdk8-src"
	files := []string{}
	filepath.WalkDir(root,
		func(path string, d fs.DirEntry, err error) error {
			files = append(files, path)
			return nil
		})
	n := 0
	for _, filename := range files {
		if n > 500 {
			break
		}
		if !strings.HasSuffix(filename, ".java") {
			continue
		}
		println(filename)
		content, err := ioutil.ReadFile(filename)
		if err != nil {
			panic("can't find file " + filename)
		}
		is := antlr.NewInputStream(string(content))
		lexer := parser.NewJavaLexer(is)

		stream := antlr.NewCommonTokenStream(lexer, antlr.TokenDefaultChannel)

		// Create the Parser
		p := parser.NewJavaParser(stream)

		p.CompilationUnit()
		n += 1
	}
	fmt.Printf("Parsed %d files\n", n)
}