REGRESSION: Searching commits can highlight wrong data points
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 31 Mar 2015 19:43:45 +0000 (19:43 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 31 Mar 2015 19:43:45 +0000 (19:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143272

Reviewed by Antti Koivisto.

The bug was caused by /api/commits returning commit times with millisecond precision whereas /api/runs
return commit times with only second precision. This resulted in the frontend code to match a commit
with the data point that included the next commit when the millisecond component of commit's timestamp
wasn't identically 0.

This discrepancy was caused by the fact PHP's strtotime only ignores milliseconds and /api/commits
was returning timestamp as string instead of parsing via Database::to_js_time as done in /api/runs
so miliseconds component was only preserved in /api/commits.

Fixed the bug by always using Database::to_js_time to return commit time. Also fixed to_js_time so that
it returns time in milisecond precision.

* public/api/commits.php:
(fetch_commits_between): Use Database::to_js_time for format commit times.
(format_commit): Ditto.
* public/include/db.php:
(Database::to_js_time): Parse and append millisecond component. Ignore sub-milliseconds for simplicity.
* public/v2/data.js:
(CommitLogs.fetchForTimeRange): The commit time is now an integer so don't call "replace" on it.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182199 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/commits.php
Websites/perf.webkit.org/public/include/db.php
Websites/perf.webkit.org/public/v2/data.js

index bf4c3a3f47498e35c5e4cfbbf95a14b3c8ec7706..ee150b376cbf190ca73f7495260493f95483786a 100644 (file)
@@ -1,3 +1,30 @@
+2015-03-31  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION: Searching commits can highlight wrong data points
+        https://bugs.webkit.org/show_bug.cgi?id=143272
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by /api/commits returning commit times with millisecond precision whereas /api/runs
+        return commit times with only second precision. This resulted in the frontend code to match a commit
+        with the data point that included the next commit when the millisecond component of commit's timestamp
+        wasn't identically 0.
+
+        This discrepancy was caused by the fact PHP's strtotime only ignores milliseconds and /api/commits
+        was returning timestamp as string instead of parsing via Database::to_js_time as done in /api/runs
+        so miliseconds component was only preserved in /api/commits.
+
+        Fixed the bug by always using Database::to_js_time to return commit time. Also fixed to_js_time so that
+        it returns time in milisecond precision.
+
+        * public/api/commits.php:
+        (fetch_commits_between): Use Database::to_js_time for format commit times.
+        (format_commit): Ditto.
+        * public/include/db.php:
+        (Database::to_js_time): Parse and append millisecond component. Ignore sub-milliseconds for simplicity.
+        * public/v2/data.js:
+        (CommitLogs.fetchForTimeRange): The commit time is now an integer so don't call "replace" on it.
+
 2015-03-31  Ryosuke Niwa  <rniwa@webkit.org>
 
         Perf dashboard should show relative change in values
index d1d79a3ef5579707ccd2c75c07d1641c672d94c2..597f716a47ff964d60986901d440aaab05eac193 100644 (file)
@@ -98,6 +98,8 @@ function fetch_commits_between($db, $repository_id, $first, $second, $keyword =
     $commits = $db->query_and_fetch_all($statements . ' ORDER BY commit_time', $values);
     if (!is_array($commits))
         exit_with_error('FailedToFetchCommits', array('repository' => $repository_id, 'first' => $first, 'second' => $second));
+    foreach ($commits as &$commit)
+        $commit['time'] = Database::to_js_time($commit['time']);
     return $commits;
 }
 
@@ -106,7 +108,7 @@ function format_commit($commit_row, $committer_row) {
         'id' => $commit_row['commit_id'],
         'revision' => $commit_row['commit_revision'],
         'parent' => $commit_row['commit_parent'],
-        'time' => $commit_row['commit_time'],
+        'time' => Database::to_js_time($commit_row['commit_time']),
         'authorName' => $committer_row ? $committer_row['committer_name'] : null,
         'authorEmail' => $committer_row ? $committer_row['committer_account'] : null,
         'message' => $commit_row['commit_message']
index 1917c094abe800ccea47bcfcaa7150bae296de85..35b5e29190499fd24d4c215c2bcbc92031a085b2 100644 (file)
@@ -65,7 +65,11 @@ class Database
     }
 
     static function to_js_time($time_str) {
-        return strtotime($time_str) * 1000;
+        $timestamp_in_s = strtotime($time_str);
+        $dot_index = strrpos($time_str, '.');
+        if ($dot_index !== FALSE)
+            $timestamp_in_s += floatval(substr($time_str, $dot_index));
+        return intval($timestamp_in_s * 1000);
     }
 
     function connect() {
index a6e4af604c23eef798c08d45f155d7daeb7d62db..dbf86a9d97b998bb615be443eb93f6e63ff1dae7 100755 (executable)
@@ -84,7 +84,7 @@ CommitLogs.fetchForTimeRange = function (repository, from, to, keyword)
             }
 
             var fetchedCommits = data.commits;
-            fetchedCommits.forEach(function (commit) { commit.time = new Date(commit.time.replace(' ', 'T')); });
+            fetchedCommits.forEach(function (commit) { commit.time = new Date(commit.time); });
 
             if (useCache)
                 CommitLogs._cacheConsecutiveCommits(repository, from, to, fetchedCommits);