Skip to content

perf: use native builtin's for foldl and foldr#629

Merged
sbarzowski merged 4 commits intogoogle:masterfrom
zendesk:jesse/native-fold-implementation
Sep 27, 2022
Merged

perf: use native builtin's for foldl and foldr#629
sbarzowski merged 4 commits intogoogle:masterfrom
zendesk:jesse/native-fold-implementation

Conversation

@Jesse-Cameron
Copy link
Copy Markdown
Contributor

👋

Description

When performing some profiling of one of our applications we found that calls to fold were quite slow. Using a native function is quite simple and gives some nice performance benefits.

Benchmark results:

benchmark                                 old ns/op      new ns/op      delta
Benchmark_Builtin_substr-8                17914126       16328579       -8.85%
Benchmark_Builtin_reverse-8               360776847      346691957      -3.90%
Benchmark_Builtin_parseInt-8              8686867        8314151        -4.29%
Benchmark_Builtin_base64Decode-8          22157223       21749268       -1.84%
Benchmark_Builtin_base64DecodeBytes-8     281975629      281841745      -0.05%
Benchmark_Builtin_base64-8                23824149       23716470       -0.45%
Benchmark_Builtin_base64_byte_array-8     141846327      141377054      -0.33%
Benchmark_Builtin_manifestJsonEx-8        9317781        9279067        -0.42%
Benchmark_Builtin_comparison-8            128783954      128362069      -0.33%
Benchmark_Builtin_comparison2-8           2082396660     2029123362     -2.56%
Benchmark_Builtin_foldl-8                 159908963      31278937       -80.44%

Note: I've left off a benchmark for foldr as I suspect it would be almost identical. Let me know if you want me to add one though.

Issue

#111

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 20, 2022

Coverage Status

Coverage increased (+0.05%) to 67.857% when pulling 523d42b on zendesk:jesse/native-fold-implementation into c450a16 on google:master.

@sbarzowski
Copy link
Copy Markdown
Contributor

Good stuff!

Would you be able to add some more testcases? Our current ones are rather rudimentary and with a native implementation we would like various cases covered, in particular:

  • Various types aggregated
  • Empty and 1 element folding
  • What if string is passed instead of an array?
    It's enough to add example code as jsonnet file(s) in testdata and then run the following to generate golden outputs:
go test -update && go test ./linter -update

(Afterwards we need to check that the results are expected.)

benchmark                                 old ns/op      new ns/op      delta
Benchmark_Builtin_substr-8                17914126       16328579       -8.85%
Benchmark_Builtin_reverse-8               360776847      346691957      -3.90%
Benchmark_Builtin_parseInt-8              8686867        8314151        -4.29%
Benchmark_Builtin_base64Decode-8          22157223       21749268       -1.84%
Benchmark_Builtin_base64DecodeBytes-8     281975629      281841745      -0.05%
Benchmark_Builtin_base64-8                23824149       23716470       -0.45%
Benchmark_Builtin_base64_byte_array-8     141846327      141377054      -0.33%
Benchmark_Builtin_manifestJsonEx-8        9317781        9279067        -0.42%
Benchmark_Builtin_comparison-8            128783954      128362069      -0.33%
Benchmark_Builtin_comparison2-8           2082396660     2029123362     -2.56%
Benchmark_Builtin_foldl-8                 159908963      31278937       -80.44%
@Jesse-Cameron Jesse-Cameron force-pushed the jesse/native-fold-implementation branch from 72e45c5 to 9c451e2 Compare September 25, 2022 08:22
@Jesse-Cameron
Copy link
Copy Markdown
Contributor Author

Jesse-Cameron commented Sep 25, 2022

Hey @sbarzowski !

Good call on improving the coverage here. It actually uncovered an edge case I hadn't covered (when the second argument is a string). I've patched this as I figure we won't want to break compatibility and added the requested tests.

LMK if there are other areas you think could benefit from a test. TY 🙇‍♂️

@sbarzowski sbarzowski merged commit b82fd4c into google:master Sep 27, 2022
@sbarzowski
Copy link
Copy Markdown
Contributor

All good, thank you!

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.

3 participants