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

Performance needs some work #133

Closed
nigeltao opened this issue Jul 25, 2018 · 6 comments
Closed

Performance needs some work #133

nigeltao opened this issue Jul 25, 2018 · 6 comments
Assignees

Comments

@nigeltao
Copy link

nigeltao commented Jul 25, 2018

According to this crude micro-benchmark on 12 made up domains, github.com/weppos/publicsuffix-go performs 460x slower than golang.org/x/net/publicsuffix:

Program:

package main

import (
	"fmt"
	"time"

	wlib "github.com/weppos/publicsuffix-go/publicsuffix"
	xlib "golang.org/x/net/publicsuffix"
)

var testCases = []string{
	"example.com",
	"example.id.au",
	"www.ck",
	"foo.bar.xn--55qx5d.cn",
	"a.b.c.minami.fukuoka.jp",
	"posts-and-telecommunications.museum",
	"www.example.pvt.k12.ma.us",
	"many.lol",
	"the.russian.for.moscow.is.xn--80adxhks",
	"blah.blah.s3-us-west-1.amazonaws.com",
	"thing.dyndns.org",
	"nosuchtld",
}

var wants = map[string]string{
	"example.com":                            "example.com",
	"example.id.au":                          "example.id.au",
	"www.ck":                                 "www.ck",
	"foo.bar.xn--55qx5d.cn":                  "bar.xn--55qx5d.cn",
	"a.b.c.minami.fukuoka.jp":                "c.minami.fukuoka.jp",
	"posts-and-telecommunications.museum":    "",
	"www.example.pvt.k12.ma.us":              "example.pvt.k12.ma.us",
	"many.lol":                               "many.lol",
	"the.russian.for.moscow.is.xn--80adxhks": "is.xn--80adxhks",
	"blah.blah.s3-us-west-1.amazonaws.com":   "blah.s3-us-west-1.amazonaws.com",
	"thing.dyndns.org":                       "thing.dyndns.org",
	"nosuchtld":                              "",
}

func main() {
	do(false)
	do(true)
}

func do(xNetLibrary bool) {
	libraryName := "github.com/weppos/publicsuffix-go"
	if xNetLibrary {
		libraryName = "golang.org/x/net/publicsuffix"
	}

	now := time.Now()
	for i := 0; i < 100; i++ {
		for _, tc := range testCases {
			got := ""
			if xNetLibrary {
				got, _ = xlib.EffectiveTLDPlusOne(tc)
			} else {
				got, _ = wlib.Domain(tc)
			}

			if i != 0 {
				continue
			}
			if want := wants[tc]; got != want {
				panic(fmt.Sprintf("xNetLibrary=%t, tc=%q: got %q, want %q",
					xNetLibrary, tc, got, want))
			}
		}
	}
	since := time.Since(now)

	fmt.Printf("%16d nanos  %s\n", since, libraryName)
}

Output:

       172856110 nanos  github.com/weppos/publicsuffix-go
          375676 nanos  golang.org/x/net/publicsuffix
@nigeltao
Copy link
Author

nigeltao commented Jul 25, 2018

This library's (github.com/weppos/publicsuffix-go/publicsuffix) performance was noticed by a colleague. There's a little more discussion at golang/go#15518 (comment)

@weppos
Copy link
Owner

weppos commented Jul 26, 2018

Thanks for opening the ticket @nigeltao. I confirm this is sort of expected due to the sequential lookup algorithm currently in use.

I'll try to allocate some time in August to switch to a more efficient lookup as mentioned in golang/go#15518 (comment)

@weppos weppos self-assigned this Aug 23, 2018
weppos added a commit that referenced this issue Aug 23, 2018
Backported from GH-143 to see the impact before/after the merge.
See also GH-133.
weppos pushed a commit that referenced this issue Aug 23, 2018
Added benchmarks, moved to a hash based implementation and improved performance to match that of golang.org/x/net library.

See GH-133
@weppos
Copy link
Owner

weppos commented Aug 23, 2018

Thanks for the work of #143 (kudos to @guliyevemil1 for contributing) performances have been drastically increased.

Before:

➜  publicsuffix-go git:(master) ✗ GOCACHE=off  go test ./... -bench=.
?   	github.com/weppos/publicsuffix-go/cmd/load	[no test files]
PASS
ok  	github.com/weppos/publicsuffix-go/net/publicsuffix	0.019s
goos: darwin
goarch: amd64
pkg: github.com/weppos/publicsuffix-go/publicsuffix
BenchmarkDomain-4   	    1000	   1522570 ns/op
BenchmarkXNet-4     	  500000	      3267 ns/op
PASS
ok  	github.com/weppos/publicsuffix-go/publicsuffix	3.383s

After

➜  publicsuffix-go git:(master) GOCACHE=off  go test ./... -bench=.
?   	github.com/weppos/publicsuffix-go/cmd/load	[no test files]
PASS
ok  	github.com/weppos/publicsuffix-go/net/publicsuffix	0.017s
goos: darwin
goarch: amd64
pkg: github.com/weppos/publicsuffix-go/publicsuffix
BenchmarkDomain-4   	  500000	      3140 ns/op
BenchmarkXNet-4     	  500000	      3313 ns/op
PASS
ok  	github.com/weppos/publicsuffix-go/publicsuffix	3.317s

@nigeltao No rush, but if you have some spare minutes, I'd appreciate your feedback whether you think I can consider this issue closed. Feel free to let me know if there's anything worth a review in the changes.

@nigeltao
Copy link
Author

I'd have to ask my colleague whether they're happy, but if you don't hear back from me in, say, a week from now, go ahead and close the issue.

@vanbroup
Copy link

I can confirm that this has improved performance significantly!
Thanks @guliyevemil1

@weppos
Copy link
Owner

weppos commented Sep 3, 2018

Thanks for the confirmation!

@weppos weppos closed this as completed Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants