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

Reorder span iterators #4754

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

Conversation

stoewer
Copy link
Contributor

@stoewer stoewer commented Feb 25, 2025

What this PR does:

Reorder iterators in createSpanIterator: move iterators of well-known attributes, intrinsic, and dedicated columns before attribute iterators (and link / event iterators).

This change shows improvements for some benchmarks.

Benchmark result
goos: linux
goarch: amd64
pkg: github.com/grafana/tempo/tempodb/encoding/vparquet4
cpu: AMD Ryzen 7 PRO 6850U with Radeon Graphics     
                                                 │ bench-traceql-02-dh-no-reorder.txt │   bench-traceql-02-dh-reorder.txt   │
                                                 │               sec/op               │   sec/op     vs base                │
BackendBlockTraceQL/spanAttVal01Match-16                                   11.43 ± 1%    11.80 ± 1%   +3.22% (p=0.000 n=10)
BackendBlockTraceQL/spanAttVal01NoMatch-16                                879.6m ± 3%   872.0m ± 1%   -0.86% (p=0.007 n=10)
BackendBlockTraceQL/spanAttVal02Match-16                                  113.7m ± 0%   114.9m ± 1%   +1.00% (p=0.019 n=10)
BackendBlockTraceQL/spanAttVal02NoMatch-16                                114.4m ± 1%   115.6m ± 1%   +1.03% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanDedicatedMatch-16                            828.9m ± 1%   834.7m ± 1%        ~ (p=0.436 n=10)
BackendBlockTraceQL/mixedSpanDedicatedNoMatch-16                          55.57m ± 1%   48.28m ± 1%  -13.12% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownMatch-16                            118.2m ± 1%   113.0m ± 0%   -4.35% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownNoMatch-16                          119.3m ± 1%   113.1m ± 1%   -5.20% (p=0.000 n=10)
geomean                                                                   309.8m        302.6m        -2.32%

                                                 │ bench-traceql-02-dh-no-reorder.txt │   bench-traceql-02-dh-reorder.txt   │
                                                 │                B/s                 │     B/s       vs base               │
BackendBlockTraceQL/spanAttVal01Match-16                                 213.3Mi ± 1%   206.7Mi ± 1%  -3.12% (p=0.000 n=10)
BackendBlockTraceQL/spanAttVal01NoMatch-16                               356.2Mi ± 2%   359.3Mi ± 1%  +0.86% (p=0.007 n=10)
BackendBlockTraceQL/spanAttVal02Match-16                                 384.1Mi ± 0%   380.3Mi ± 1%  -0.99% (p=0.019 n=10)
BackendBlockTraceQL/spanAttVal02NoMatch-16                               381.8Mi ± 1%   377.9Mi ± 1%  -1.02% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanDedicatedMatch-16                           667.3Mi ± 1%   662.7Mi ± 1%       ~ (p=0.436 n=10)
BackendBlockTraceQL/mixedSpanDedicatedNoMatch-16                         671.5Mi ± 1%   646.9Mi ± 1%  -3.67% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownMatch-16                           421.1Mi ± 1%   386.4Mi ± 0%  -8.24% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownNoMatch-16                         417.3Mi ± 1%   386.4Mi ± 1%  -7.41% (p=0.000 n=10)
geomean                                                                  414.9Mi        402.1Mi       -3.08%

                                                 │ bench-traceql-02-dh-no-reorder.txt │    bench-traceql-02-dh-reorder.txt    │
                                                 │              MB_io/op              │  MB_io/op    vs base                  │
BackendBlockTraceQL/spanAttVal01Match-16                                  2.557k ± 0%   2.557k ± 0%        ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttVal01NoMatch-16                                 328.5 ± 0%    328.5 ± 0%        ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttVal02Match-16                                   45.81 ± 0%    45.81 ± 0%        ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/spanAttVal02NoMatch-16                                 45.81 ± 0%    45.81 ± 0%        ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedSpanDedicatedMatch-16                             580.0 ± 0%    580.0 ± 0%        ~ (p=1.000 n=10) ¹
BackendBlockTraceQL/mixedSpanDedicatedNoMatch-16                           39.13 ± 0%    32.75 ± 0%  -16.30% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownMatch-16                             52.19 ± 0%    45.81 ± 0%  -12.22% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownNoMatch-16                           52.19 ± 0%    45.81 ± 0%  -12.22% (p=0.000 n=10)
geomean                                                                    134.8         127.6        -5.34%
¹ all samples are equal

                                                 │ bench-traceql-02-dh-no-reorder.txt │   bench-traceql-02-dh-reorder.txt    │
                                                 │                B/op                │     B/op      vs base                │
BackendBlockTraceQL/spanAttVal01Match-16                                 1.442Gi ± 5%   1.364Gi ± 8%        ~ (p=0.123 n=10)
BackendBlockTraceQL/spanAttVal01NoMatch-16                               1.034Gi ± 5%   1.035Gi ± 3%        ~ (p=0.579 n=10)
BackendBlockTraceQL/spanAttVal02Match-16                                 126.0Mi ± 1%   126.2Mi ± 1%        ~ (p=0.631 n=10)
BackendBlockTraceQL/spanAttVal02NoMatch-16                               126.3Mi ± 1%   125.9Mi ± 1%        ~ (p=0.529 n=10)
BackendBlockTraceQL/mixedSpanDedicatedMatch-16                           618.5Mi ± 4%   628.7Mi ± 6%        ~ (p=0.436 n=10)
BackendBlockTraceQL/mixedSpanDedicatedNoMatch-16                         58.82Mi ± 1%   37.46Mi ± 1%  -36.30% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownMatch-16                           145.0Mi ± 1%   125.4Mi ± 1%  -13.51% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownNoMatch-16                         145.4Mi ± 1%   126.1Mi ± 0%  -13.31% (p=0.000 n=10)
geomean                                                                  257.1Mi        233.2Mi        -9.28%

                                                 │ bench-traceql-02-dh-no-reorder.txt │  bench-traceql-02-dh-reorder.txt   │
                                                 │             allocs/op              │  allocs/op   vs base               │
BackendBlockTraceQL/spanAttVal01Match-16                                  785.4k ± 0%   785.1k ± 0%       ~ (p=0.342 n=10)
BackendBlockTraceQL/spanAttVal01NoMatch-16                                143.0k ± 0%   143.0k ± 0%       ~ (p=0.912 n=10)
BackendBlockTraceQL/spanAttVal02Match-16                                  140.6k ± 0%   140.6k ± 0%  -0.00% (p=0.024 n=10)
BackendBlockTraceQL/spanAttVal02NoMatch-16                                140.6k ± 0%   140.6k ± 0%       ~ (p=0.118 n=10)
BackendBlockTraceQL/mixedSpanDedicatedMatch-16                            182.1k ± 0%   182.1k ± 0%       ~ (p=0.436 n=10)
BackendBlockTraceQL/mixedSpanDedicatedNoMatch-16                          140.7k ± 0%   140.7k ± 0%  -0.01% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownMatch-16                            140.8k ± 0%   140.7k ± 0%  -0.04% (p=0.000 n=10)
BackendBlockTraceQL/mixedSpanWellKnownNoMatch-16                          140.8k ± 0%   140.7k ± 0%  -0.04% (p=0.000 n=10)
geomean                                                                   180.5k        180.5k       -0.01%

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@stoewer stoewer changed the title Reorder iterators [WIP] Reorder iterators Feb 26, 2025
@stoewer stoewer changed the title Reorder iterators Reorder span iterators Feb 26, 2025
@stoewer stoewer marked this pull request as ready for review February 26, 2025 04:29
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Another nice one!

What do you think about revisiting the BenchmarkBackendBlockTraceQL to reduce the number of queries and to make them better represent a range of real world query patterns?

It seems like you made some adjustments in this PR and I'd love to see the queries used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants