Re: [PATCH] Revert "gitweb: Time::HiRes is in core for Perl 5.8"

On Fri, 27 Jan 2012, Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

Though Time::HiRes is a core Perl module, it doesn't necessarily mean
that it is included in 'perl' package, and that it is installed if
Perl is installed.

I do not think we have seen the end of Redhat/Fedora Perl saga.  I am
hoping that either one of the two things to happen:

 (1) Redhat/Fedora distrubution reconsiders the situation and fix their
     packages so that by default when its users ask for "Perl" they get
     what the upstream distributes as "Perl" in full, while still allowing
     people who know what they are doing to install a minimum subset
     "perl-base"; or

 (2) Many applications that use and rely on Perl like we do are hit by
     this issue, and Redhat/Fedora users are trained to install the
     perl-full (or whatever it is called) package when applications want
     "Perl".

In other words, I am hoping that "it doesn't necessarily mean" will not
stay true for a long time.  So please hold onto this patch until the dust
settles, and resend it if (1) does not look to be happening in say 3
months.

 
So for the time being (those "3 months") you would apply instead your
change to INSTALL (or equivalent to gitweb/INSTALL) mentioning Time::HiRes
issue, and perhaps also original patch by Hallvard skipping gitweb tests
if Time::HiRes is not available, isn't it?
 

quoted

For example RedHat has split it out to a separate RPM perl-Time-HiRes.

[...]

quoted

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index abb5a79..c86224a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -17,10 +17,12 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
-use Time::HiRes qw(gettimeofday tv_interval);
 binmode STDOUT, ':utf8';
 
-our $t0 = [ gettimeofday() ];
+our $t0;
+if (eval { require Time::HiRes; 1; }) {
+	$t0 = [Time::HiRes::gettimeofday()];
+}
 our $number_of_git_cmds = 0;

Why should these even be initialized here?  Doesn't reset_timer gets
called at the beginning of run_request()?

I think it predates adding reset_timer() to gitweb.  Anyway $t0 has
to be set to something defined anyway to denote that Time::HiRes is
available... though if Time::HiRes is required unconditionally it would
not be really needed, and we can always check $INC{'Time/HiRes.pm'}
if it was loaded or not.

quoted

 BEGIN {

@@ -1142,7 +1144,7 @@ sub dispatch {
 }
 
 sub reset_timer {
-	our $t0 = [ gettimeofday() ]
+	our $t0 = [Time::HiRes::gettimeofday()]
 		if defined $t0;
 	our $number_of_git_cmds = 0;

The statement modifier look ugly.

More importantly, if you are not profiling, i.e. if we didn't initialize
$t0 at the beginning, do you need to reset $number_of_git_cmds at all?

I also think this should take gitweb_check_feature('timed') into
account, perhaps like this:

	sub reset_timer {
        	return unless gitweb_check_feature('timed');
                our $t0 = ...
                our $number_of_git_cmds = 0;
	}

Then all the other

	if (defined $t0 && gitweb_check_feature('timed'))

can become

	if (defined $t0)

I think this is a good idea... though it would complicate applying revert
a bit ;-(

If you go this route, even though tee-zero, the beginning of the time, is
a good name for the variable, you may want to rename it to avoid confusing
readers who might take it as a temporary variable #0.

Good idea.  $request_start_time perhaps?  Or $time_start?

-- 
Jakub Narebski
Poland