Re: [PATCH i-g-t 2/4] scripts/trace.pl: Sort order

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx