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

⚡ Speedup merge #65

Merged
merged 1 commit into from Dec 8, 2022
Merged

⚡ Speedup merge #65

merged 1 commit into from Dec 8, 2022

Conversation

AdityaVallabh
Copy link
Contributor

@AdityaVallabh AdityaVallabh commented Nov 8, 2022

The Merge function currently checks if a key in other exists in p by iterating over all keys in p which is redundant as we already have a faster way to look that up in p.m.

This reduces the asymptotic complexity of Merge from quadratic O(N2) down to linear O(N) where N is the number of properties.

Also wrote a benchmark to measure the exact speedup and here are the results:

Merging hundred properties - 6x faster
Merging thousand properties - 37x faster
Merging 10K properties. - 300x faster
Merging 100K properties - 2600x faster

Before

$ GOMAXPROCS=1 go test -run=^$ -bench ^BenchmarkMerge$ github.com/magiconair/properties
goos: linux
goarch: amd64
pkg: github.com/magiconair/properties
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkMerge/num_properties_100                  58174             19691 ns/op
BenchmarkMerge/num_properties_1000                   687           1637600 ns/op
BenchmarkMerge/num_properties_10000                    7         156907466 ns/op
BenchmarkMerge/num_properties_100000                   1        16630149940 ns/op
PASS

After

$ GOMAXPROCS=1 go test -run=^$ -bench ^BenchmarkMerge$ github.com/magiconair/properties
goos: linux
goarch: amd64
pkg: github.com/magiconair/properties
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkMerge/num_properties_100                 342036              3254 ns/op
BenchmarkMerge/num_properties_1000                 27264             44239 ns/op
BenchmarkMerge/num_properties_10000                 2245            523613 ns/op
BenchmarkMerge/num_properties_100000                 190           6237122 ns/op
PASS

Copy link
Contributor Author

@AdityaVallabh AdityaVallabh left a comment

Choose a reason for hiding this comment

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

@magiconair can you please take a look?

@magiconair
Copy link
Owner

Yes, I'll try to review and merge today or tomorrow.

@magiconair magiconair merged commit 44ba29f into magiconair:main Dec 8, 2022
@magiconair
Copy link
Owner

Done. Merged to v1.8.7

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

2 participants