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

starlark: built-in types map of *Builtin #264

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Mar 5, 2020

NewBuiltin and Sort are called at init. Slightly faster method calls:

benchmark                            old ns/op     new ns/op     delta
BenchmarkStringHash/hard-1-8         3.90          3.65          -6.41%
BenchmarkStringHash/soft-1-8         2.69          2.21          -17.84%
BenchmarkStringHash/hard-2-8         4.52          4.38          -3.10%
BenchmarkStringHash/soft-2-8         3.55          2.68          -24.51%
BenchmarkStringHash/hard-4-8         5.32          5.07          -4.70%
BenchmarkStringHash/soft-4-8         5.32          3.63          -31.77%
BenchmarkStringHash/hard-8-8         7.58          7.03          -7.26%
BenchmarkStringHash/soft-8-8         6.62          5.60          -15.41%
BenchmarkStringHash/hard-16-8        7.00          6.43          -8.14%
BenchmarkStringHash/soft-16-8        13.1          9.44          -27.94%
BenchmarkStringHash/hard-32-8        7.28          7.12          -2.20%
BenchmarkStringHash/soft-32-8        21.7          21.2          -2.30%
BenchmarkStringHash/hard-64-8        9.13          8.75          -4.16%
BenchmarkStringHash/soft-64-8        50.9          54.1          +6.29%
BenchmarkStringHash/hard-128-8       13.0          12.8          -1.54%
BenchmarkStringHash/soft-128-8       114           112           -1.75%
BenchmarkStringHash/hard-256-8       15.7          15.3          -2.55%
BenchmarkStringHash/soft-256-8       241           235           -2.49%
BenchmarkStringHash/hard-512-8       23.7          23.2          -2.11%
BenchmarkStringHash/soft-512-8       492           483           -1.83%
BenchmarkStringHash/hard-1024-8      38.8          38.5          -0.77%
BenchmarkStringHash/soft-1024-8      987           981           -0.61%
BenchmarkHashtable-8                 2152734       2163910       +0.52%
Benchmark/bench_bigint-8             271664        269095        -0.95%
Benchmark/bench_builtin_method-8     392849        354319        -9.81%
Benchmark/bench_calling-8            643982        638527        -0.85%
Benchmark/bench_int-8                124893        123253        -1.31%
Benchmark/bench_range-8              308           301           -2.27%
BenchmarkProgram/read-8              17762         17676         -0.48%
BenchmarkProgram/compile-8           269868        268148        -0.64%
BenchmarkProgram/encode-8            11632         11434         -1.70%
BenchmarkProgram/decode-8            13839         13554         -2.06%
BenchmarkScan-8                      632375        629915        -0.39%
BenchmarkParse-8                     1310575       1305860       -0.36%

@emcfarlane emcfarlane changed the title Builtins remove method closure & sort starlark: builtins remove method closure & sort Mar 5, 2020
Copy link
Contributor

@alandonovan alandonovan left a comment

Choose a reason for hiding this comment

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

Thanks for this change. I'm curious: do you actually need faster method calls, or was this just for fun?

The code looks sound. However, I wonder if it's not a little more complex than necessary. Specifically, I don't think we need to optimize the AttrNames operation, which is used only for dir(x), whose performance is not so important, I think. If we revert it to the old AttrNames approach of allocating a new slice and sorting the map keys, then the remainder of the change is the simplification of method closures to avoid the two allocations (one for the unnecessary change-of-signature wrapper, and one for Builtin). We could then replace 'struct builtins' with a simple map[string]*Builtin.

Thoughts?

@emcfarlane
Copy link
Contributor Author

Just for fun! I noticed the double allocation while trying to implement built ins. I’m happy to go with the simplified *Builtin.

starlark/library.go Outdated Show resolved Hide resolved
@emcfarlane emcfarlane changed the title starlark: builtins remove method closure & sort starlark: built-in types map of *Builtin Mar 6, 2020
Copy link
Contributor

@alandonovan alandonovan left a comment

Choose a reason for hiding this comment

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

I like this even better. Thanks!

@alandonovan alandonovan merged commit 8dd3e2e into google:master Mar 6, 2020
@emcfarlane emcfarlane deleted the builtins branch March 7, 2020 02:41
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.

2 participants