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

chore(perf): optimise tm_alltrue() and tm_anytrue(). #1282

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented Dec 6, 2023

What this PR does / why we need it:

PROPOSAL: This PR optimises tm_alltrue() and tm_anytrue but introduces a non-breaking behavior change that could be undesirable, so it requires discussion.

It optimises the tm_alltrue() and tm_anytrue() by lazy evaluating the function's arguments using the customdecode.ExpressionClosureType and then only evaluating the needed argument elements as below:
For tm_alltrue() the function returns as soon as a false element is found.
For tm_anytrue() the function returns as soon as a true element is found.

Benchmark results:

time/op:
TmAllTrueLiteralList-4: old 5.55ms ± 0%: new 0.00ms ± 1%: delta: -99.97%
TmAllTrueFuncall-4: old 143µs ± 1%: new 142µs ± 0%: delta: -0.81%
TmAnyTrueLiteralList-4: old 129ms ± 0%: new 0ms ± 1%: delta: -100.00%
TmAnyTrueFuncall-4: old 143µs ± 1%: new 142µs ± 1%: delta: -1.08%

alloc/op:
TmAllTrueLiteralList-4: old 1.74MB ± 0%: new 0.00MB ± 0%: delta: -99.98%
TmAllTrueFuncall-4: old 45.5kB ± 0%: new 44.9kB ± 0%: delta: -1.37%
TmAnyTrueLiteralList-4: old 37.9MB ± 0%: new 0.0MB ± 0%: delta: -100.00%
TmAnyTrueFuncall-4: old 45.6kB ± 0%: new 45.0kB ± 0%: delta: -1.37%

allocs/op:
TmAllTrueLiteralList-4: old 13.6k ± 0%: new 0.0k ± 0%: delta: -99.95%
TmAllTrueFuncall-4: old 460 ± 0%: new 440 ± 0%: delta: -4.35%
TmAnyTrueLiteralList-4: old 252k ± 0%: new 0k ± 0%: delta: -100.00%
TmAnyTrueFuncall-4: old 462 ± 0%: new 442 ± 0%: delta: -4.33%

The TmAllTrueLiteralList* benchmark tests the case where expensive expressions are provided in the function arguments, then the expensive part is not evaluated, that's why the huge performance win.

Which issue(s) this PR fixes:

Special notes for your reviewer:

This implementation does not match 100% the original, see below:

before:
tm_alltrue(false, tm_unknown_func()) // evaluation error: no function called tm_unknown_func

after:
tm_alltrue(false, tm_unknown_func()) // returns false

Does this PR introduce a user-facing change?

yes, now some errors can be not reported. The behavior is yet to be approved.

Checklist

  • Document the behavior change in both functions.

Copy link

github-actions bot commented Dec 6, 2023

metric: time/op
ChangeDetection-4: old 3.96ms ± 4%: new 3.96ms ± 6%: delta: 0.00%
ChangeDetectionTFAndTG-4: old 13.8ms ±10%: new 12.6ms ± 3%: delta: -8.86%
CloudReadLines-4: old 1.01ms ± 7%: new 1.00ms ± 7%: delta: 0.00%
CloudReadLine-4: old 7.13ms ± 3%: new 7.18ms ± 1%: delta: 0.00%
ListFiles-4: old 80.0µs ± 1%: new 80.6µs ± 2%: delta: 0.83%
Generate-4: old 1.56s ± 5%: new 1.56s ± 3%: delta: 0.00%
GenerateRegex-4: old 1.03s ± 4%: new 1.06s ± 5%: delta: 2.00%
TokensForExpressionComplex-4: old 1.25ms ± 0%: new 1.25ms ± 0%: delta: 0.00%
TokensForExpressionPlainStringNoNewline-4: old 789ns ± 1%: new 790ns ± 1%: delta: 0.00%
TokensForExpressionStringWith100Newlines-4: old 23.6µs ± 2%: new 23.6µs ± 1%: delta: 0.00%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 1.44ms ± 0%: new 1.44ms ± 1%: delta: 0.26%
TokensForExpression-4: old 1.25ms ± 0%: new 1.25ms ± 1%: delta: 0.18%
PartialEvalComplex-4: old 537µs ± 1%: new 533µs ± 0%: delta: -0.64%
PartialEvalSmallString-4: old 3.87µs ± 0%: new 3.88µs ± 0%: delta: 0.16%
PartialEvalHugeString-4: old 1.90ms ± 1%: new 1.96ms ± 4%: delta: 3.07%
PartialEvalHugeInterpolatedString-4: old 4.94ms ± 2%: new 4.97ms ± 3%: delta: 0.00%
PartialEvalObject-4: old 26.8µs ± 1%: new 26.6µs ± 1%: delta: -0.73%
TmAllTrueLiteralList-4: old 616µs ± 2%: new 1µs ± 1%: delta: -99.80%
TmAllTrueFuncall-4: old 20.2µs ± 1%: new 19.4µs ± 1%: delta: -4.21%
TmAnyTrueLiteralList-4: old 4.82ms ± 0%: new 0.00ms ± 0%: delta: -99.97%
TmAnyTrueFuncall-4: old 20.6µs ± 1%: new 19.4µs ± 0%: delta: -6.07%
TmTernary-4: old 2.51µs ± 0%: new 2.51µs ± 1%: delta: 0.00%
TmTryUnknownFunc-4: old 2.27µs ± 1%: new 2.28µs ± 1%: delta: 0.29%
TmTryUnknownVariable-4: old 2.17µs ± 1%: new 2.19µs ± 2%: delta: 1.13%
TmTryUnknownObjectKey-4: old 2.48µs ± 0%: new 2.49µs ± 1%: delta: 0.28%
metric: alloc/op
ChangeDetection-4: old 357kB ± 0%: new 357kB ± 0%: delta: 0.01%
ChangeDetectionTFAndTG-4: old 4.80MB ± 0%: new 4.80MB ± 0%: delta: 0.00%
CloudReadLines-4: old 3.12MB ± 0%: new 3.12MB ± 0%: delta: 0.00%
CloudReadLine-4: old 3.37MB ± 0%: new 3.37MB ± 0%: delta: 0.00%
ListFiles-4: old 27.6kB ± 0%: new 27.6kB ± 0%: delta: 0.00%
Generate-4: old 2.24GB ± 0%: new 2.24GB ± 0%: delta: 0.00%
GenerateRegex-4: old 926MB ± 0%: new 926MB ± 0%: delta: 0.00%
TokensForExpressionComplex-4: old 394kB ± 0%: new 394kB ± 0%: delta: 0.00%
TokensForExpressionPlainStringNoNewline-4: old 512B ± 0%: new 512B ± 0%: delta: 0.00%
TokensForExpressionStringWith100Newlines-4: old 12.5kB ± 0%: new 12.5kB ± 0%: delta: 0.00%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 395kB ± 0%: new 395kB ± 0%: delta: 0.00%
TokensForExpression-4: old 394kB ± 0%: new 394kB ± 0%: delta: 0.00%
PartialEvalComplex-4: old 361kB ± 0%: new 361kB ± 0%: delta: 0.00%
PartialEvalSmallString-4: old 1.95kB ± 0%: new 1.95kB ± 0%: delta: 0.00%
PartialEvalHugeString-4: old 196kB ± 0%: new 196kB ± 0%: delta: 0.00%
PartialEvalHugeInterpolatedString-4: old 4.31MB ± 0%: new 4.31MB ± 0%: delta: 0.00%
PartialEvalObject-4: old 19.4kB ± 0%: new 19.4kB ± 0%: delta: 0.00%
TmAllTrueLiteralList-4: old 321kB ± 0%: new 0kB ± 0%: delta: -99.91%
TmAllTrueFuncall-4: old 10.6kB ± 0%: new 10.0kB ± 0%: delta: -6.47%
TmAnyTrueLiteralList-4: old 2.09MB ± 0%: new 0.00MB ± 0%: delta: -99.99%
TmAnyTrueFuncall-4: old 10.7kB ± 0%: new 10.0kB ± 0%: delta: -6.43%
TmTernary-4: old 1.18kB ± 0%: new 1.18kB ± 0%: delta: 0.00%
TmTryUnknownFunc-4: old 784B ± 0%: new 784B ± 0%: delta: 0.00%
TmTryUnknownVariable-4: old 768B ± 0%: new 768B ± 0%: delta: 0.00%
TmTryUnknownObjectKey-4: old 952B ± 0%: new 952B ± 0%: delta: 0.00%
metric: allocs/op
ChangeDetection-4: old 2.43k ± 0%: new 2.43k ± 0%: delta: 0.01%
ChangeDetectionTFAndTG-4: old 33.6k ± 0%: new 33.6k ± 0%: delta: -0.06%
CloudReadLines-4: old 5.54k ± 0%: new 5.54k ± 0%: delta: 0.00%
CloudReadLine-4: old 60.0k ± 0%: new 60.0k ± 0%: delta: 0.00%
ListFiles-4: old 335 ± 0%: new 335 ± 0%: delta: 0.00%
Generate-4: old 25.7M ± 0%: new 25.7M ± 0%: delta: 0.00%
GenerateRegex-4: old 18.4M ± 0%: new 18.4M ± 0%: delta: 0.00%
TokensForExpressionComplex-4: old 4.83k ± 0%: new 4.83k ± 0%: delta: 0.00%
TokensForExpressionPlainStringNoNewline-4: old 20.0 ± 0%: new 20.0 ± 0%: delta: 0.00%
TokensForExpressionStringWith100Newlines-4: old 227 ± 0%: new 227 ± 0%: delta: 0.00%
TokensForExpressionObjectWith100KeysWithNumberValues-4: old 3.19k ± 0%: new 3.19k ± 0%: delta: 0.00%
TokensForExpression-4: old 4.83k ± 0%: new 4.83k ± 0%: delta: 0.00%
PartialEvalComplex-4: old 3.78k ± 0%: new 3.78k ± 0%: delta: 0.00%
PartialEvalSmallString-4: old 26.0 ± 0%: new 26.0 ± 0%: delta: 0.00%
PartialEvalHugeString-4: old 38.0 ± 0%: new 38.0 ± 0%: delta: 0.00%
PartialEvalHugeInterpolatedString-4: old 26.1k ± 0%: new 26.1k ± 0%: delta: 0.00%
PartialEvalObject-4: old 183 ± 0%: new 183 ± 0%: delta: 0.00%
TmAllTrueLiteralList-4: old 5.94k ± 0%: new 0.01k ± 0%: delta: -99.87%
TmAllTrueFuncall-4: old 275 ± 0%: new 254 ± 0%: delta: -7.64%
TmAnyTrueLiteralList-4: old 59.6k ± 0%: new 0.0k ± 0%: delta: -99.99%
TmAnyTrueFuncall-4: old 277 ± 0%: new 256 ± 0%: delta: -7.58%
TmTernary-4: old 27.0 ± 0%: new 27.0 ± 0%: delta: 0.00%
TmTryUnknownFunc-4: old 21.0 ± 0%: new 21.0 ± 0%: delta: 0.00%
TmTryUnknownVariable-4: old 20.0 ± 0%: new 20.0 ± 0%: delta: 0.00%
TmTryUnknownObjectKey-4: old 23.0 ± 0%: new 23.0 ± 0%: delta: 0.00%

Base automatically changed from i4k-add-benchmark-tm-all-true to main December 6, 2023 17:53
Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for docs-terramate-io canceled.

Name Link
🔨 Latest commit a819810
🔍 Latest deploy log https://app.netlify.com/sites/docs-terramate-io/deploys/6570b62242d5ad0008cafb77

@i4ki i4ki marked this pull request as ready for review December 6, 2023 18:24
@i4ki i4ki requested a review from a team as a code owner December 6, 2023 18:24
Copy link
Contributor

@wmalik wmalik left a comment

Choose a reason for hiding this comment

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

Nice one!

@soerenmartius
Copy link
Contributor

Nice one!

really well done @i4ki

@mariux mariux marked this pull request as draft January 3, 2024 15:22
@i4ki i4ki changed the base branch from main to i4k-fix-race October 23, 2024 19:36
Copy link

github-actions bot commented Oct 23, 2024

2024/10/23 19:58:46 Terraform detected version: 1.9.7
toolsetPath: /tmp/cmd-terramate-test-4213473174
=== RUN   TestInteropCloudSyncPreview
=== RUN   TestInteropCloudSyncPreview/preview:_basic-drift
    interoperability_test.go:46: using GITHUB_EVENT_FILE=/home/runner/work/_temp/_github_workflow/event.json
=== RUN   TestInteropCloudSyncPreview/preview:_basic-drift-uppercase-id
    interoperability_test.go:46: using GITHUB_EVENT_FILE=/home/runner/work/_temp/_github_workflow/event.json
--- PASS: TestInteropCloudSyncPreview (11.16s)
    --- PASS: TestInteropCloudSyncPreview/preview:_basic-drift (5.82s)
    --- PASS: TestInteropCloudSyncPreview/preview:_basic-drift-uppercase-id (5.34s)
=== RUN   TestInteropSyncDeployment
=== RUN   TestInteropSyncDeployment/deployment:_empty
=== RUN   TestInteropSyncDeployment/deployment:_empty-uppercase-id
--- PASS: TestInteropSyncDeployment (36.77s)
    --- PASS: TestInteropSyncDeployment/deployment:_empty (18.46s)
    --- PASS: TestInteropSyncDeployment/deployment:_empty-uppercase-id (18.31s)
=== RUN   TestInteropDrift
=== RUN   TestInteropDrift/drift:_basic-drift
=== RUN   TestInteropDrift/drift:_basic-drift-uppercase-id
--- PASS: TestInteropDrift (59.91s)
    --- PASS: TestInteropDrift/drift:_basic-drift (29.71s)
    --- PASS: TestInteropDrift/drift:_basic-drift-uppercase-id (30.20s)
PASS
ok  	github.com/terramate-io/terramate/e2etests/cloud/interop	119.032s

Copy link

github-actions bot commented Oct 23, 2024

Preview of ubuntu-focal/go1.21 tests in cf4a062

🔍 View Details on Terramate Cloud

globals
stdlib

@i4ki i4ki marked this pull request as ready for review October 23, 2024 20:01
@i4ki i4ki requested a review from a team October 23, 2024 20:01
Copy link

Preview of macos-ventura/go1.21 tests in cf4a062

🔍 View Details on Terramate Cloud

globals
stdlib

zied-elouaer
zied-elouaer previously approved these changes Oct 24, 2024
snakster
snakster previously approved these changes Oct 24, 2024
Base automatically changed from i4k-fix-race to main October 24, 2024 12:20
@i4ki i4ki dismissed stale reviews from snakster and zied-elouaer October 24, 2024 12:20

The base branch was changed.

Copy link
Contributor

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants