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:Microsoft Windows
Assigned to: CPU Architecture:Any
Tags: Contribution, profiling, windows
Triage: Needs Triage: D5 (Feature request)

[4 Jan 2010 16:44] Lenz Grimmer
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
[5 Jan 2010 15:42] Matthew Bradbury
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;
}
[5 Jan 2010 23:12] Alex Budovski
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.
[5 Jan 2010 23:28] Alex Budovski
Updated patch

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

[3 Feb 2010 2:50] Reggie Burnett
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.
[4 Feb 2010 14:24] Vladislav Vaintroub
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.
[4 Feb 2010 22:50] Alex Budovski
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!
[5 Feb 2010 1:44] Alex Budovski
Revised patch.

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

[5 Feb 2010 12:57] Bugs System
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.
[5 Feb 2010 12:57] Bugs System
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.
[5 Feb 2010 16:32] Bugs System
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.
[5 Feb 2010 17:18] Vladislav Vaintroub
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!
[13 Feb 2010 8:36] Bugs System
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)
[13 Feb 2010 8:39] Bugs System
Pushed into mysql-next-mr (revid:alik@sun.com-20100213083327-cee4ao3jpg33eggv) (version source revid:luis.soares@sun.com-20100211135018-1f9dbghg0itszigo) (pib:16)
[13 Feb 2010 17:51] Paul Dubois
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.
[6 Mar 2010 10:59] Bugs System
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)
[7 Mar 2010 1:19] Paul Dubois
Noted in 5.5.3 changelog.