[2/4] scripts/trace.pl: Sort order

Submitted by John Harrison on Jan. 20, 2018, 12:24 a.m.

Details

Message ID 20180120002421.13919-3-John.C.Harrison@Intel.com
State New
Series "scripts/trace.pl: Re-order calculations and fixups"
Headers show

Commit Message

John Harrison Jan. 20, 2018, 12:24 a.m.
From: John Harrison <John.C.Harrison@Intel.com>

Add an extra level to the databse key sort so that the ordering is
deterministic. If the time stamp matches, it now compares the key
itself as well (context/seqno). This makes it much easier to determine
if a change has actually broken anything. Previously back to back runs
with no changes could still produce different output, especially when
adding extra debug output during the calculations.

As the comparison test is now more than a single equation, moved it
out into a separate sort function.

Signed-off-by: John Harrison <John.C.Harrison@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 scripts/trace.pl | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/scripts/trace.pl b/scripts/trace.pl
index 7b8a920e..cf841b7e 100755
--- a/scripts/trace.pl
+++ b/scripts/trace.pl
@@ -540,7 +540,25 @@  my (%submit_avg, %execute_avg, %ctxsave_avg);
 my $last_ts = 0;
 my $first_ts;
 
-my @sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db;
+sub sortStart {
+	my $as = $db{$a}->{'start'};
+	my $bs = $db{$b}->{'start'};
+
+	return $as <=> $bs if($as ne $bs);
+
+	return $a cmp $b;
+}
+
+sub sortQueue {
+	my $as = $db{$a}->{'queue'};
+	my $bs = $db{$b}->{'queue'};
+
+	return $as <=> $bs if($as ne $bs);
+
+	return $a cmp $b;
+}
+
+my @sorted_keys = sort sortStart keys %db;
 my $re_sort = 0;
 die "Database changed size?!" unless scalar(@sorted_keys) == $keyCount;
 
@@ -589,9 +607,9 @@  foreach my $key (@sorted_keys) {
 	$ctxsave_avg{$ring} += $db{$key}->{'end'} - $db{$key}->{'notify'};
 }
 
-@sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db if $re_sort;
+@sorted_keys = sort sortStart keys %db if $re_sort;
 
-foreach my $ring (keys %batch_avg) {
+foreach my $ring (sort keys %batch_avg) {
 	$batch_avg{$ring} /= $batch_count{$ring};
 	$batch_total_avg{$ring} /= $batch_count{$ring};
 	$submit_avg{$ring} /= $batch_count{$ring};
@@ -831,7 +849,7 @@  print <<ENDHTML;
 ENDHTML
 
 my $i = 0;
-foreach my $key (sort {$db{$a}->{'queue'} <=> $db{$b}->{'queue'}} keys %db) {
+foreach my $key (sort sortQueue keys %db) {
 	my ($name, $ctx, $seqno) = ($db{$key}->{'name'}, $db{$key}->{'ctx'}, $db{$key}->{'seqno'});
 	my ($queue, $start, $notify, $end) = ($db{$key}->{'queue'}, $db{$key}->{'start'}, $db{$key}->{'notify'}, $db{$key}->{'end'});
 	my $submit = $queue + $db{$key}->{'submit-delay'};

Comments

Tvrtko Ursulin Jan. 22, 2018, 10:19 a.m.
On 20/01/2018 00:24, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Add an extra level to the databse key sort so that the ordering is
> deterministic. If the time stamp matches, it now compares the key
> itself as well (context/seqno). This makes it much easier to determine
> if a change has actually broken anything. Previously back to back runs
> with no changes could still produce different output, especially when
> adding extra debug output during the calculations.

Makes sense. I guess I never expected ns resolution time stamps to be 
different.

> As the comparison test is now more than a single equation, moved it
> out into a separate sort function.
> 
> Signed-off-by: John Harrison <John.C.Harrison@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   scripts/trace.pl | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/trace.pl b/scripts/trace.pl
> index 7b8a920e..cf841b7e 100755
> --- a/scripts/trace.pl
> +++ b/scripts/trace.pl
> @@ -540,7 +540,25 @@ my (%submit_avg, %execute_avg, %ctxsave_avg);
>   my $last_ts = 0;
>   my $first_ts;
>   
> -my @sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db;
> +sub sortStart {
> +	my $as = $db{$a}->{'start'};
> +	my $bs = $db{$b}->{'start'};
> +
> +	return $as <=> $bs if($as ne $bs);

In the spirit of the last round of optimising work, I would check if 
this is the most optimal way to write this. Perhaps it would be better 
with a single comparison on this line. And not mixing string and 
numerical context? Like:

	my $val = $as <=> $bs;

	$val = $a cmp $b if $val == 0;

	return $val;

?

> +
> +	return $a cmp $b;
> +}
> +
> +sub sortQueue {
> +	my $as = $db{$a}->{'queue'};
> +	my $bs = $db{$b}->{'queue'};
> +
> +	return $as <=> $bs if($as ne $bs);
> +
> +	return $a cmp $b;
> +}
> +
> +my @sorted_keys = sort sortStart keys %db;
>   my $re_sort = 0;
>   die "Database changed size?!" unless scalar(@sorted_keys) == $keyCount;
>   
> @@ -589,9 +607,9 @@ foreach my $key (@sorted_keys) {
>   	$ctxsave_avg{$ring} += $db{$key}->{'end'} - $db{$key}->{'notify'};
>   }
>   
> -@sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db if $re_sort;
> +@sorted_keys = sort sortStart keys %db if $re_sort;
>   
> -foreach my $ring (keys %batch_avg) {
> +foreach my $ring (sort keys %batch_avg) {
>   	$batch_avg{$ring} /= $batch_count{$ring};
>   	$batch_total_avg{$ring} /= $batch_count{$ring};
>   	$submit_avg{$ring} /= $batch_count{$ring};
> @@ -831,7 +849,7 @@ print <<ENDHTML;
>   ENDHTML
>   
>   my $i = 0;
> -foreach my $key (sort {$db{$a}->{'queue'} <=> $db{$b}->{'queue'}} keys %db) {
> +foreach my $key (sort sortQueue keys %db) {
>   	my ($name, $ctx, $seqno) = ($db{$key}->{'name'}, $db{$key}->{'ctx'}, $db{$key}->{'seqno'});
>   	my ($queue, $start, $notify, $end) = ($db{$key}->{'queue'}, $db{$key}->{'start'}, $db{$key}->{'notify'}, $db{$key}->{'end'});
>   	my $submit = $queue + $db{$key}->{'submit-delay'};
> 

Rest looks OK.

Regards,

Tvrtko
John Harrison Jan. 22, 2018, 10:51 p.m.
On 1/22/2018 2:19 AM, Tvrtko Ursulin wrote:
>
> On 20/01/2018 00:24, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Add an extra level to the databse key sort so that the ordering is
>> deterministic. If the time stamp matches, it now compares the key
>> itself as well (context/seqno). This makes it much easier to determine
>> if a change has actually broken anything. Previously back to back runs
>> with no changes could still produce different output, especially when
>> adding extra debug output during the calculations.
>
> Makes sense. I guess I never expected ns resolution time stamps to be 
> different.

Computers are fast - ns is slow!

>
>> As the comparison test is now more than a single equation, moved it
>> out into a separate sort function.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   scripts/trace.pl | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/trace.pl b/scripts/trace.pl
>> index 7b8a920e..cf841b7e 100755
>> --- a/scripts/trace.pl
>> +++ b/scripts/trace.pl
>> @@ -540,7 +540,25 @@ my (%submit_avg, %execute_avg, %ctxsave_avg);
>>   my $last_ts = 0;
>>   my $first_ts;
>>   -my @sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} 
>> keys %db;
>> +sub sortStart {
>> +    my $as = $db{$a}->{'start'};
>> +    my $bs = $db{$b}->{'start'};
>> +
>> +    return $as <=> $bs if($as ne $bs);
>
> In the spirit of the last round of optimising work, I would check if 
> this is the most optimal way to write this. Perhaps it would be better 
> with a single comparison on this line. And not mixing string and 
> numerical context? Like:
>
>     my $val = $as <=> $bs;
>
>     $val = $a cmp $b if $val == 0;
>
>     return $val;
>
> ?
Six of one, half a dozen of another. Again, I would assume the compiler 
would be smart enough to make both versions equal but I'm happy to go 
with your version if you feel it is better.

>
>> +
>> +    return $a cmp $b;
>> +}
>> +
>> +sub sortQueue {
>> +    my $as = $db{$a}->{'queue'};
>> +    my $bs = $db{$b}->{'queue'};
>> +
>> +    return $as <=> $bs if($as ne $bs);
>> +
>> +    return $a cmp $b;
>> +}
>> +
>> +my @sorted_keys = sort sortStart keys %db;
>>   my $re_sort = 0;
>>   die "Database changed size?!" unless scalar(@sorted_keys) == 
>> $keyCount;
>>   @@ -589,9 +607,9 @@ foreach my $key (@sorted_keys) {
>>       $ctxsave_avg{$ring} += $db{$key}->{'end'} - $db{$key}->{'notify'};
>>   }
>>   -@sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} 
>> keys %db if $re_sort;
>> +@sorted_keys = sort sortStart keys %db if $re_sort;
>>   -foreach my $ring (keys %batch_avg) {
>> +foreach my $ring (sort keys %batch_avg) {
>>       $batch_avg{$ring} /= $batch_count{$ring};
>>       $batch_total_avg{$ring} /= $batch_count{$ring};
>>       $submit_avg{$ring} /= $batch_count{$ring};
>> @@ -831,7 +849,7 @@ print <<ENDHTML;
>>   ENDHTML
>>     my $i = 0;
>> -foreach my $key (sort {$db{$a}->{'queue'} <=> $db{$b}->{'queue'}} 
>> keys %db) {
>> +foreach my $key (sort sortQueue keys %db) {
>>       my ($name, $ctx, $seqno) = ($db{$key}->{'name'}, 
>> $db{$key}->{'ctx'}, $db{$key}->{'seqno'});
>>       my ($queue, $start, $notify, $end) = ($db{$key}->{'queue'}, 
>> $db{$key}->{'start'}, $db{$key}->{'notify'}, $db{$key}->{'end'});
>>       my $submit = $queue + $db{$key}->{'submit-delay'};
>>
>
> Rest looks OK.
>
> Regards,
>
> Tvrtko