[1/4] scripts/trace.pl: More hash key optimisations

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

Details

Message ID 20180120002421.13919-2-John.C.Harrison@Intel.com
State New
Headers show
Series "scripts/trace.pl: Re-order calculations and fixups" ( rev: 1 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

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

Cache the key count value rather than querying the hash every time.
Also assert that the database does not magically change size after the
fixups.

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

Patch hide | download patch | download mbox

diff --git a/scripts/trace.pl b/scripts/trace.pl
index cb93900d..7b8a920e 100755
--- a/scripts/trace.pl
+++ b/scripts/trace.pl
@@ -508,7 +508,9 @@  foreach my $key (keys %db) {
 }
 
 # Fix up incompletes
-foreach my $key (keys %db) {
+my @keys = keys(%db);
+my $keyCount = scalar(@keys);
+foreach my $key (@keys) {
 	next unless exists $db{$key}->{'incomplete'};
 
 	# End the incomplete batch at the time next one starts
@@ -522,7 +524,7 @@  foreach my $key (keys %db) {
 		$next_key = db_key($ring, $ctx, $seqno + $i);
 		$i++;
 	} until ((exists $db{$next_key} and not exists $db{$next_key}->{'incomplete'})
-		 or $i > scalar(keys(%db)));  # ugly stop hack
+		 or $i > $keyCount);  # ugly stop hack
 
 	if (exists $db{$next_key}) {
 		$db{$key}->{'notify'} = $db{$next_key}->{'end'};
@@ -540,6 +542,7 @@  my $first_ts;
 
 my @sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db;
 my $re_sort = 0;
+die "Database changed size?!" unless scalar(@sorted_keys) == $keyCount;
 
 foreach my $key (@sorted_keys) {
 	my $ring = $db{$key}->{'ring'};
@@ -565,7 +568,7 @@  foreach my $key (@sorted_keys) {
 		do {
 			$next_key = db_key($ring, $ctx, $seqno + $i);
 			$i++;
-		} until (exists $db{$next_key} or $i > scalar(keys(%db)));  # ugly stop hack
+		} until (exists $db{$next_key} or $i > $keyCount);  # ugly stop hack
 
 		# 20us tolerance
 		if (exists $db{$next_key} and $db{$next_key}->{'start'} < $start + 20) {

Comments

On 20/01/2018 00:24, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Cache the key count value rather than querying the hash every time.

This actually makes a difference? Just curious, I would have assumed 
Perl would know the size of it's arrays but maybe the implementation is 
stupid...

Btw I was using Devel::NTYProf in HTML output mode to profile my 
changes. It is quite easy to use and provided all the info I needed.

> Also assert that the database does not magically change size after the
> fixups.
> 
> Signed-off-by: John Harrison <John.C.Harrison@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   scripts/trace.pl | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/trace.pl b/scripts/trace.pl
> index cb93900d..7b8a920e 100755
> --- a/scripts/trace.pl
> +++ b/scripts/trace.pl
> @@ -508,7 +508,9 @@ foreach my $key (keys %db) {
>   }
>   
>   # Fix up incompletes
> -foreach my $key (keys %db) {
> +my @keys = keys(%db);

This array is unused except for the query below so I'd suggest to not 
have it.

> +my $keyCount = scalar(@keys);

About came case.. I won't complain too the max, since I'm happy you are 
finding the tool useful and improving it, but it would be good to stay 
within the same style of variable naming or we'll have a mish-mash of 
styles which won't help readability.

Regards,

Tvrtko

> +foreach my $key (@keys) {
>   	next unless exists $db{$key}->{'incomplete'};
>   
>   	# End the incomplete batch at the time next one starts
> @@ -522,7 +524,7 @@ foreach my $key (keys %db) {
>   		$next_key = db_key($ring, $ctx, $seqno + $i);
>   		$i++;
>   	} until ((exists $db{$next_key} and not exists $db{$next_key}->{'incomplete'})
> -		 or $i > scalar(keys(%db)));  # ugly stop hack
> +		 or $i > $keyCount);  # ugly stop hack
>   
>   	if (exists $db{$next_key}) {
>   		$db{$key}->{'notify'} = $db{$next_key}->{'end'};
> @@ -540,6 +542,7 @@ my $first_ts;
>   
>   my @sorted_keys = sort {$db{$a}->{'start'} <=> $db{$b}->{'start'}} keys %db;
>   my $re_sort = 0;
> +die "Database changed size?!" unless scalar(@sorted_keys) == $keyCount;
>   
>   foreach my $key (@sorted_keys) {
>   	my $ring = $db{$key}->{'ring'};
> @@ -565,7 +568,7 @@ foreach my $key (@sorted_keys) {
>   		do {
>   			$next_key = db_key($ring, $ctx, $seqno + $i);
>   			$i++;
> -		} until (exists $db{$next_key} or $i > scalar(keys(%db)));  # ugly stop hack
> +		} until (exists $db{$next_key} or $i > $keyCount);  # ugly stop hack
>   
>   		# 20us tolerance
>   		if (exists $db{$next_key} and $db{$next_key}->{'start'} < $start + 20) {
>
On 1/22/2018 2:14 AM, Tvrtko Ursulin wrote:
>
> On 20/01/2018 00:24, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Cache the key count value rather than querying the hash every time.
>
> This actually makes a difference? Just curious, I would have assumed 
> Perl would know the size of it's arrays but maybe the implementation 
> is stupid...

Actually, I'm not sure it does anymore. I thought it did but I later 
decided that the change was actually just run to run noise. However, I 
already had the patch and I think it makes the code at least look 
simpler. IMHO, '$key_count' is easier to read than 'scalar(keys(%db))' 
and it is obviously trivial rather than relying on the compiler to be smart.

>
> Btw I was using Devel::NTYProf in HTML output mode to profile my 
> changes. It is quite easy to use and provided all the info I needed.
>
>> Also assert that the database does not magically change size after the
>> fixups.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   scripts/trace.pl | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/trace.pl b/scripts/trace.pl
>> index cb93900d..7b8a920e 100755
>> --- a/scripts/trace.pl
>> +++ b/scripts/trace.pl
>> @@ -508,7 +508,9 @@ foreach my $key (keys %db) {
>>   }
>>     # Fix up incompletes
>> -foreach my $key (keys %db) {
>> +my @keys = keys(%db);
>
> This array is unused except for the query below so I'd suggest to not 
> have it.
>
>> +my $keyCount = scalar(@keys);
>
> About came case.. I won't complain too the max, since I'm happy you 
> are finding the tool useful and improving it, but it would be good to 
> stay within the same style of variable naming or we'll have a 
> mish-mash of styles which won't help readability.

Oops, I missed that. Habit and too many different style guides in too 
many different projects! I'll change it to $key_count instead.


>
> Regards,
>
> Tvrtko
>
>> +foreach my $key (@keys) {
>>       next unless exists $db{$key}->{'incomplete'};
>>         # End the incomplete batch at the time next one starts
>> @@ -522,7 +524,7 @@ foreach my $key (keys %db) {
>>           $next_key = db_key($ring, $ctx, $seqno + $i);
>>           $i++;
>>       } until ((exists $db{$next_key} and not exists 
>> $db{$next_key}->{'incomplete'})
>> -         or $i > scalar(keys(%db)));  # ugly stop hack
>> +         or $i > $keyCount);  # ugly stop hack
>>         if (exists $db{$next_key}) {
>>           $db{$key}->{'notify'} = $db{$next_key}->{'end'};
>> @@ -540,6 +542,7 @@ my $first_ts;
>>     my @sorted_keys = sort {$db{$a}->{'start'} <=> 
>> $db{$b}->{'start'}} keys %db;
>>   my $re_sort = 0;
>> +die "Database changed size?!" unless scalar(@sorted_keys) == $keyCount;
>>     foreach my $key (@sorted_keys) {
>>       my $ring = $db{$key}->{'ring'};
>> @@ -565,7 +568,7 @@ foreach my $key (@sorted_keys) {
>>           do {
>>               $next_key = db_key($ring, $ctx, $seqno + $i);
>>               $i++;
>> -        } until (exists $db{$next_key} or $i > scalar(keys(%db)));  
>> # ugly stop hack
>> +        } until (exists $db{$next_key} or $i > $keyCount); # ugly 
>> stop hack
>>             # 20us tolerance
>>           if (exists $db{$next_key} and $db{$next_key}->{'start'} < 
>> $start + 20) {
>>