maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Kristian!

On Jul 31, Kristian Nielsen wrote:
> Hi Serg,
> 
> Can you review below patch? (sorry for no commit mail, I forgot to configure
> outgoing mail on my new laptop before committing).
> 
> I was for a long time annoyed that mtr takes 20 seconds starting up before it
> starts running the first test. So being bored one evening I ran a Perl
> profiler on the code and fixed the culprit.

Ah, nice!
Thanks.

Just one question and couple of suggestions below.
If you agree with them - please change accordingly and push!

> diff:
> === modified file 'mysql-test/lib/mtr_cases.pm'
> --- mysql-test/lib/mtr_cases.pm	2011-05-02 17:58:45 +0000
> +++ mysql-test/lib/mtr_cases.pm	2011-07-31 08:40:10 +0000
> @@ -890,7 +890,8 @@
>      if ( -f "$testdir/$tname.slave-mi");
>  
>  
> -  my @source_files = tags_from_test_file($tinfo,"$testdir/${tname}.test");
> +  my ($master_opts, $slave_opts)=
> +    tags_from_test_file($tinfo,"$testdir/${tname}.test");
>  
>    # Get default storage engine from suite.opt file
>  
> @@ -1043,16 +1044,8 @@
>    # ----------------------------------------------------------------------
>    # Append mysqld extra options to master and slave, as appropriate
>    # ----------------------------------------------------------------------
> -  for (@source_files) {
> -    s/\.\w+$//;
> -    push @{$tinfo->{master_opt}}, opts_from_file("$_.opt");
> -    push @{$tinfo->{slave_opt}}, opts_from_file("$_.opt");
> -    push @{$tinfo->{master_opt}}, opts_from_file("$_-master.opt");
> -    push @{$tinfo->{slave_opt}}, opts_from_file("$_-slave.opt");
> -  }
> -
> -  push(@{$tinfo->{'master_opt'}}, @::opt_extra_mysqld_opt);
> -  push(@{$tinfo->{'slave_opt'}}, @::opt_extra_mysqld_opt);
> +  push @{$tinfo->{'master_opt'}}, @$master_opts, @::opt_extra_mysqld_opt;
> +  push @{$tinfo->{'slave_opt'}}, @$slave_opts, @::opt_extra_mysqld_opt;
>  
>    process_opts($tinfo, 'master_opt');
>    process_opts($tinfo, 'slave_opt');
> @@ -1061,73 +1054,106 @@
>  }
>  
>  
> -# List of tags in the .test files that if found should set
> -# the specified value in "tinfo"
> -my @tags=
> -(
> - ["include/big_test.inc", "big_test", 1],
> - ["include/have_debug.inc", "need_debug", 1],
> - ["include/have_ndb.inc", "ndb_test", 1],
> - ["include/have_multi_ndb.inc", "ndb_test", 1],
> - ["include/master-slave.inc", "rpl_test", 1],
> - ["include/ndb_master-slave.inc", "rpl_test", 1],
> - ["include/ndb_master-slave.inc", "ndb_test", 1],
> - ["include/not_embedded.inc", "not_embedded", 1],
> - ["include/not_valgrind.inc", "not_valgrind", 1],
> - ["include/have_example_plugin.inc", "example_plugin_test", 1],
> - ["include/have_ssl.inc", "need_ssl", 1],
> - ["include/long_test.inc", "long_test", 1],
> -);
> -
> +my $tags_map= {'big_test' => ['big_test', 1],
> +               'have_debug' => ['need_debug', 1],
> +               'have_ndb' => ['ndb_test', 1],
> +               'have_multi_ndb' => ['ndb_test', 1],
> +               'master-slave' => ['rpl_test', 1],
> +               'ndb_master-slave' => ['rpl_test', 1, 'ndb_test', 1],
> +               'not_embedded' => ['not_embedded', 1],
> +               'not_valgrind' => ['not_valgrind', 1],
> +               'have_example_plugin' => ['example_plugin_test', 1],
> +               'have_oqgraph_engine' => ['oqgraph_test', 1],
> +               'have_ssl' => ['need_ssl', 1],
> +               'long_test' => ['long_test', 1],
> +};
>
> +my $tags_regex_string= join('|', keys %$tags_map);
> +my $tags_regex= qr:include/($tags_regex_string)\.inc:o;
> +
> +my $file_to_tags= { };
> +my $file_to_master_opts= { };
> +my $file_to_slave_opts= { };
> +
> +# Get various tags from a file, recursively scanning also included files.
> +# And get options from .opt file, also recursively for included files.
> +# Return a list of [TAG_TO_SET, VALUE_TO_SET_TO] of found tags.
> +# Also returns lists of options for master and slave found in .opt files.
> +# Each include file is scanned only once, and subsequent calls just look up the
> +# cached result.
> +# We need to be a bit careful about speed here; previous version of this code
> +# took forever to scan the full test suite.
> +sub get_tags_from_file {
> +  my ($file, $base_dir)= @_;
> +
> +  return ($file_to_tags->{$file}, $file_to_master_opts->{$file},
> +          $file_to_slave_opts->{$file})
> +    if exists($file_to_tags->{$file});
> +
> +  my $F= IO::File->new($file)
> +    or mtr_error("can't open file \"$file\": $!");
> +  $base_dir= dirname($file)
> +    unless defined($base_dir);

why do you pass base_dir as an argument, instead of always using
basedir($file) ?

> +  my $tags= [];
> +  my $file_no_ext= $file;
> +  $file_no_ext =~ s/\.\w+$//;
> +  my @common_opts= opts_from_file("$file_no_ext.opt");
> +  my $master_opts= [@common_opts, opts_from_file("$file_no_ext-master.opt")];
> +  my $slave_opts= [@common_opts, opts_from_file("$file_no_ext-slave.opt")];
> +
> +  while (my $line= <$F>)
> +  {
> +    # Ignore comments.
> +    next if $line =~ /^\#/;
> +
> +    # Add any tag we find.
> +    if ($line =~ /$tags_regex/o)
> +    {
> +      my $to_set= $tags_map->{$1};
> +      for (my $i= 0; $i < @$to_set; $i+= 2)
> +      {
> +        push @$tags, [$to_set->[$i], $to_set->[$i+1]];
> +      }
> +    }
> +
> +    # Check for a sourced include file.
> +    if ($line =~ /^(--)?[[:space:]]*source[[:space:]]+([^;[:space:]]+)/)
> +    {
> +      my $include= $2;
> +      # Sourced file may exist relative to test file, or in global location.
> +      # Note that we ignore non-existing files (eg. mysqltest.test needs this)

I'd clarify here "Note that for the purpose of tag collection we ignore
non-existing files, and let mysqltest handle the error (e.g.
mysqltest.test needs this)"

> +      for my $sourced_file ($base_dir . '/' . $include,
> +                            $::glob_mysql_test_dir . '/' . $include)

Can you also look in the suite dir ? In the order of
  basedir(file), suite_dir, glob_mysql_test_dir.

> +      {
> +        if (-e $sourced_file)
> +        {
> +          my ($sub_tags, $sub_master_opts, $sub_slave_opts)=
> +            get_tags_from_file($sourced_file, $base_dir);
> +          push @$tags, @$sub_tags;
> +          # Tests (rpl.rpl_ddl at least) rely on options being in reverse order
> +          # of include :-/ Hence the unshift().
> +          unshift @$master_opts, @$sub_master_opts;
> +          unshift @$slave_opts, @$sub_slave_opts;

I think having options in the order of includes is more logical.
Better to sort includes in the rpl_ddl.test, and use push here.
It is more intuitive and less error-prone.

Note that MySQL's mtr does not look for .opt files recursively at all,
it was my extension. Feel free to make options to follow the include
order :)

> +          last;
> +        }
> +      }
> +    }
> +  }
> +  $file_to_tags->{$file}= $tags;
> +  $file_to_master_opts->{$file}= $master_opts;
> +  $file_to_slave_opts->{$file}= $slave_opts;
> +  return ($tags, $master_opts, $slave_opts);
> +}
>  
>  sub tags_from_test_file {
> -  my $tinfo= shift;
> -  my $file= shift;
> -  #mtr_verbose("$file");
> -  my $F= IO::File->new($file) or mtr_error("can't open file \"$file\": $!");
> -  my @all_files=($file);
> +  my ($tinfo, $file)= @_;
>  
> -  while ( my $line= <$F> )
> +  my ($tags, $master_opts, $slave_opts)= get_tags_from_file($file);
> +  for (@$tags)
>    {
> -
> -    # Skip line if it start's with #
> -    next if ( $line =~ /^#/ );
> -
> -    # Match this line against tag in "tags" array
> -    foreach my $tag (@tags)
> -    {
> -      if ( index($line, $tag->[0]) >= 0 )
> -      {
> -	# Tag matched, assign value to "tinfo"
> -	$tinfo->{"$tag->[1]"}= $tag->[2];
> -      }
> -    }
> -
> -    # If test sources another file, open it as well
> -    if ( $line =~ /^\-\-([[:space:]]*)source(.*)$/ or
> -	 $line =~ /^([[:space:]]*)source(.*);$/ )
> -    {
> -      my $value= $2;
> -      $value =~ s/^\s+//;  # Remove leading space
> -      $value =~ s/[[:space:]]+$//;  # Remove ending space
> -
> -      # Sourced file may exist relative to test or
> -      # in global location
> -      foreach my $sourced_file (dirname($file). "/$value",
> -				"$::glob_mysql_test_dir/$value")
> -      {
> -	if ( -f $sourced_file )
> -	{
> -	  # Only source the file if it exists, we may get
> -	  # false positives in the regexes above if someone
> -	  # writes "source nnnn;" in a test case(such as mysqltest.test)
> -	  unshift @all_files, tags_from_test_file($tinfo, $sourced_file);
> -	  last;
> -	}
> -      }
> -    }
> +    $tinfo->{$_->[0]}= $_->[1];
>    }
> -  @all_files;
> +  return ($master_opts, $slave_opts);
>  }
>  
>  sub unspace {
> 
> === modified file 'mysql-test/suite/innodb_plugin/t/innodb_bug52663.test'
> --- mysql-test/suite/innodb_plugin/t/innodb_bug52663.test	2010-04-26 10:27:25 +0000
> +++ mysql-test/suite/innodb_plugin/t/innodb_bug52663.test	2011-07-31 08:40:10 +0000
> @@ -1,5 +1,7 @@
>  --source include/have_innodb_plugin.inc
>  
> +--source include/long_test.inc
> +
>  set session transaction isolation level read committed;
>  
>  create table innodb_bug52663 (what varchar(5), id integer, count integer, primary key

Regards,
Sergei