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

Update publicsuffix-go to 6f3c5059 #3826

Merged
merged 2 commits into from
Aug 24, 2018
Merged

Conversation

weppos
Copy link
Contributor

@weppos weppos commented Aug 23, 2018

Sorry for the extra noise, but thanks to external contributions the lookup has been drastically improved and I've been working with some consumers to double check everything is working as expected.

Despite I carefully inspected the changes, I want to make sure no subtle use cases are broken.

If you are interested in, here's the benchmarks:
weppos/publicsuffix-go#133 (comment)

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

Performance are now equivalent to the golang/x package, still providing the flexibility that was the core of the design of my alternate version.

If you want to inspect the changes, the PR is weppos/publicsuffix-go#143


As per contribution rule, tests are green:

➜  publicsuffix-go git:(master) GOCACHE=off  go test ./...
?   	github.com/weppos/publicsuffix-go/cmd/load	[no test files]
ok  	github.com/weppos/publicsuffix-go/net/publicsuffix	0.021s
ok  	github.com/weppos/publicsuffix-go/publicsuffix	0.021s

Incorporate performance improvements.
@weppos weppos requested a review from a team as a code owner August 23, 2018 12:54
@weppos
Copy link
Contributor Author

weppos commented Aug 23, 2018

I can see the specs failed.

--- FAIL: TestExtractDomainIANASuffix_Valid (0.00s)
	pa_test.go:446: "kobe.jp": got "kobe.jp", want "jp"
FAIL
FAIL	github.com/letsencrypt/boulder/policy	0.038s

I'm looking into the case.

@weppos weppos changed the title Update publicsuffix-go to b6e96f6 Update publicsuffix-go to 6f3c5059 Aug 23, 2018
@weppos
Copy link
Contributor Author

weppos commented Aug 23, 2018

There was an edge case that I was able to discover thanks to your test suite. I fixed the issue in weppos/publicsuffix-go@f8afde6, added regression tests to the lib test suite, and now also LE tests are green.

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @weppos, I really appreciate that you found the time to backport this update to Boulder. Nice to hear our unit tests were useful as well!!

I scanned the diff on the upstream library and it looks good to me.

@cpu cpu merged commit 36a1ded into letsencrypt:master Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants