MySQL Bugs: #50057: PATCH: SHOW PROFILE CPU port for Windows

Bug #50057	PATCH: SHOW PROFILE CPU port for Windows
Submitted:	4 Jan 2010 16:44	Modified:	7 Mar 2010 1:19
Reporter:	Lenz Grimmer	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Windows	Severity:	S4 (Feature request)
Version:	5.4	OS:	Windows
Assigned to:		CPU Architecture:	Any
Tags:	Contribution, profiling, windows

Description:
A patch contribution by Alex Budovski, posted to the internals mailing list:

  http://lists.mysql.com/internals/37616

(The attachment contains a bzr merge directive)

Here is a small patch that ports SHOW PROFILE CPU to Windows.

=== modified file 'sql/sql_profile.cc'
--- sql/sql_profile.cc	2009-02-13 16:41:47 +0000
+++ sql/sql_profile.cc	2009-12-23 00:32:41 +0000
@@ -134,6 +134,20 @@
 #define RUSAGE_USEC(tv)  ((tv).tv_sec*1000*1000 + (tv).tv_usec)
 #define RUSAGE_DIFF_USEC(tv1, tv2) (RUSAGE_USEC((tv1))-RUSAGE_USEC((tv2)))
 
+#ifdef __WIN__
+// Adapted from "Programming Applications for Microsoft Windows".
+inline ULONGLONG FileTimeToQuadWord(FILETIME *ft)
+{
+  return (Int64ShllMod32(ft->dwHighDateTime, 32) | ft->dwLowDateTime);
+}
+
+
+// Get time difference between to FILETIME objects in seconds.
+inline double GetTimeDiffInSeconds(FILETIME *a, FILETIME *b)
+{
+  return ((FileTimeToQuadWord(a) - FileTimeToQuadWord(b)) / 1e7);
+}
+#endif
 
 PROF_MEASUREMENT::PROF_MEASUREMENT(QUERY_PROFILE *profile_arg, const char
                                    *status_arg)
@@ -224,6 +238,9 @@
   time_usecs= (double) my_getsystime() / 10.0;  /* 1 sec was 1e7, now is 1e6 */
 #ifdef HAVE_GETRUSAGE
   getrusage(RUSAGE_SELF, &rusage);
+#elif defined(__WIN__)
+  FILETIME ftDummy;
+  GetProcessTimes(GetCurrentProcess(), &ftDummy, &ftDummy, &ftKernel, &ftUser);
 #endif
 }
 
@@ -593,6 +610,23 @@
       table->field[5]->store_decimal(&cpu_stime_decimal);
       table->field[4]->set_notnull();
       table->field[5]->set_notnull();
+#elif defined(__WIN__)
+      my_decimal cpu_utime_decimal, cpu_stime_decimal;
+
+      double2my_decimal(E_DEC_FATAL_ERROR,
+                        GetTimeDiffInSeconds(&entry->ftUser,
+                                             &previous->ftUser),
+                        &cpu_utime_decimal);
+      double2my_decimal(E_DEC_FATAL_ERROR,
+                        GetTimeDiffInSeconds(&entry->ftKernel,
+                                             &previous->ftKernel),
+                        &cpu_stime_decimal);
+
+      // Store the result.
+      table->field[4]->store_decimal(&cpu_utime_decimal);
+      table->field[5]->store_decimal(&cpu_stime_decimal);
+      table->field[4]->set_notnull();
+      table->field[5]->set_notnull();
 #else
       /* TODO: Add CPU-usage info for non-BSD systems */
 #endif

=== modified file 'sql/sql_profile.h'
--- sql/sql_profile.h	2007-12-14 13:57:37 +0000
+++ sql/sql_profile.h	2009-12-23 00:32:41 +0000
@@ -186,6 +186,8 @@
   char *status;
 #ifdef HAVE_GETRUSAGE
   struct rusage rusage;
+#elif defined(__WIN__)
+  FILETIME ftKernel, ftUser;
 #endif
 
   char *function;

How to repeat:
Apply the patch, check that SHOW PROFILE CPU now works on Windows

You have a subtle bug there, the second parameter of Int64ShllMod32 can only 
be in the range of 0 - 31.
So perhaps a better version would be:

inline ULONGLONG FileTimeToQuadWord(FILETIME *ft)
{
  ULONGLONG nrv = 0;
  nrv |= ft->dwHighDateTime;
  nrv <<= 32;
  nrv |= ft->dwLowDateTime;
  return nrv;
}

Good catch! Looks like either the book has an error, or the documentation is wrong. I'll make the change to your version, using the regular bit shift operator.

Updated patch

Attachment: cpu1.patch (application/octet-stream, text), 5.58 KiB.

Looks ok to me but I've also assigned Vlad to review it.  He may catch some things that I miss.  Thanks for submitting this.

Hi Alex,
this looks good, I only have very minor points to it (which you are free to ignore, if you wish, and then I'll push it as-is). They are only style-type comments.

1) Using __WIN__ . Since long time already we're trying to get of __WIN__ and replace it with _WIN32, an least in the new code . Reason being that __WIN__ is completely artificial creature and violates ANSI which reserves identifiers starting with double underscore for compiler writers.

2) Maybe it is simpler instead of bit twiddling in the conversion of FILETIME to ULONGLONG to use ULARGE_INTEGER union, as also suggested here http://msdn.microsoft.com/en-us/library/ms724284(VS.85).aspx

3)
inline ULONGLONG FileTimeToQuadWord()
inline static double GetTimeDiffInSeconds()

Why inline instead of e.g static? Does not look like we would need to care  much about performance of those 2 functions.

Hi Vladislav!

Excellent points!

Re:
1) I agree completely. I used __WIN__ merely to be consistent with the surrounding code. But I too prefer using the standard _WIN32 macro.

2) I guess a union could have worked. I'll see what I can do.

3) Only reason for inline is that I figured it wasn't worth paying the cost of entry and return for those 2 simple functions. But I guess they won't be called often enough to matter?

Static is a good choice, as presumably they won't be needed elsewhere.

I'll make the changes.

Thanks again!

Revised patch.

Attachment: cpu.patch (text/x-diff), 2.55 KiB.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/99411

2986 Vladislav Vaintroub	2010-02-05
      Bug#50057: 'SHOW PROFILE CPU' port for Windows.
      Patch contributed by Alex Budkovski.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/99412

2986 Vladislav Vaintroub	2010-02-05
      Bug#50057: 'SHOW PROFILE CPU' port for Windows.
      Patch contributed by Alex Budovski.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/99485

2987 Vladislav Vaintroub	2010-02-05
      Bug#50057: SHOW PROFILE CPU for Windows
      
      On QA request, adding test that causes new code to be called. 
      Even if we cannot validate the result, this will at least increase the code coverage.

Alex, I pushed your latest patch plus some test code that QA asked for
- that was only for code coverage, since a meaningful test  for SHOW PROFILES CPU with checking results is not possible.

Thank you for contribution!

Pushed into 6.0.14-alpha (revid:alik@sun.com-20100213083436-9pesg4h55w1mekxc) (version source revid:luis.soares@sun.com-20100211135109-t63avry9fqpgyh78) (merge vers: 6.0.14-alpha) (pib:16)

Pushed into mysql-next-mr (revid:alik@sun.com-20100213083327-cee4ao3jpg33eggv) (version source revid:luis.soares@sun.com-20100211135018-1f9dbghg0itszigo) (pib:16)

Noted in 6.0.14 changelog.

SHOW PROFILE CPU has been ported to Windows. 

Setting report to Need Merge pending push of Celosia to release tree.

Pushed into 5.5.3-m3 (revid:alik@sun.com-20100306103849-hha31z2enhh7jwt3) (version source revid:vvaintroub@mysql.com-20100213160132-nx1vlocxuta76txh) (merge vers: 5.5.99-m3) (pib:16)

Noted in 5.5.3 changelog.