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.

Sign up for GitHub

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

Jump to bottom

Do not report unused aggregations #3

Closed
euloh opened this issue Jan 7, 2021 · 1 comment
Closed

Do not report unused aggregations #3

euloh opened this issue Jan 7, 2021 · 1 comment
Assignees
@euloh

Comments

@euloh
Copy link
Member

euloh commented Jan 7, 2021

Aggregations may be described in a D script but never used (e.g., probes using them never fire). In this case, the aggregations should not be reported.

Consider

        BEGIN {
            x = 1234;
            @a = sum(x);
            exit(0)
        }
        tick-7s {
            @b = min(x);
            @c = max(x)
        }

Aggregation @A will have data, but @b and @c will not. Here is the behavior with DTv1:

                    1234

That is, the sum() aggregation is reported while the min() and max() aggregations, having no data, are not. In contrast, here is DTv2:

        DTrace 2.0.0 [Pre-Release with limited functionality]
                        1234
         9223372036854775807
         -9223372036854775808

Here, the min() and max() aggregations are reported as well.
@euloh euloh self-assigned this Jan 7, 2021
euloh added a commit that referenced this issue Jan 14, 2021
@euloh
Switch from per-CPU latches to per-CPU/per-agg latches
ee132c7 
Change the latch sequence number to be per-aggregation, not only per-CPU.
This way, one can see whether an aggregation has any data or not.  (Some
aggregations, like count(), already inherently have such information, but
we need to handle the general case.)

This change has the added advantage of reducing producer-consumer conflicts.
E.g., imagine that the producer is firing very often, updating different
aggregations, while the consuming is trying to read a single, large
aggregation (e.g., some *quantize aggregation).  A single latch sequence
number for all aggregations would keep updating, causing the consumer to
keep rereading an aggregation unnecessarily.

Also, when taking snapshots of aggregations, copy only a single copy of
the aggregation data.  Neither the latch sequence number nor the second
copy are needed.

This also solves the problem that aggregations that appear in a D script
were being printed even if the probes that use them never fire.

Introduce a test to catch the printing of unused aggregations.

#3
Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
alan-maguire pushed a commit to alan-maguire/dtrace-utils that referenced this issue Aug 19, 2021
@euloh @kvanhees
Change dt_cg_agg_lquantize() arg check to agree with message
5db20ff 
We check limitval<baseval, but the error message is "base must
be less than limit".  Make the check more stringent to agree
with the error message.

Note that this behavior dates back to DTrace v1:
     # dtrace -n 'BEGIN {@ = lquantize(8, 4, 4); exit(0) }'
     dtrace: invalid probe specifier:
     lquantize( ) step (argument oracle#3) too large:
     must have at least one quantization level
That is, the arguments pass the check but are then caught by a
later, rather confusing, message.

Add a test to catch this case.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
@kvanhees
Copy link
Member

This has been resolved in an earlier release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@euloh euloh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@euloh @kvanhees