From c13f9d157b125cdab5a41dd8b30d3346e69dbfd4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 2 Apr 2013 06:26:26 -0700 Subject: [PATCH] Fix double-stops on profiler sampling Summary: For some time, we've stopped the profiler twice when it was invoked by the sampling mechanism. The first time it actually stops, and we write a profile. The second time it hadn't been started, so it returns empty and we write an invalid profile. Instead, keep track of whether it is running or not, and don't stop it a second time. Ref T2870. Test Plan: Set sample rate to 1-in-3, observed valid sample profiles generate. Reviewers: btrahan, chad CC: aran Maniphest Tasks: T2870 Differential Revision: https://secure.phabricator.com/D5534 --- src/__celerity_resource_map__.php | 189 ++++++++---------- .../plugin/DarkConsoleXHProfPlugin.php | 14 +- .../xhprof/DarkConsoleXHProfPluginAPI.php | 57 ++++-- webroot/index.php | 1 - 4 files changed, 131 insertions(+), 130 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 9d2f240dbd..359461a95e 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1189,15 +1189,14 @@ celerity_register_resource_map(array( ), 'javelin-behavior-aphront-drag-and-drop-textarea' => array( - 'uri' => '/res/853e33b9/rsrc/js/application/core/behavior-drag-and-drop-textarea.js', + 'uri' => '/res/326d6c1c/rsrc/js/application/core/behavior-drag-and-drop-textarea.js', 'type' => 'js', 'requires' => array( 0 => 'javelin-behavior', 1 => 'javelin-dom', 2 => 'phabricator-drag-and-drop-file-upload', - 3 => 'phabricator-paste-file-upload', - 4 => 'phabricator-textareautils', + 3 => 'phabricator-textareautils', ), 'disk' => '/rsrc/js/application/core/behavior-drag-and-drop-textarea.js', ), @@ -2879,7 +2878,7 @@ celerity_register_resource_map(array( ), 'phabricator-drag-and-drop-file-upload' => array( - 'uri' => '/res/a4c31f49/rsrc/js/application/core/DragAndDropFileUpload.js', + 'uri' => '/res/df7582fd/rsrc/js/application/core/DragAndDropFileUpload.js', 'type' => 'js', 'requires' => array( @@ -3106,20 +3105,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/application/objectselector/object-selector.css', ), - 'phabricator-paste-file-upload' => - array( - 'uri' => '/res/b0b8afd8/rsrc/js/application/core/PasteFileUpload.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-install', - 1 => 'javelin-util', - 2 => 'javelin-request', - 3 => 'javelin-dom', - 4 => 'javelin-uri', - ), - 'disk' => '/rsrc/js/application/core/PasteFileUpload.js', - ), 'phabricator-pinboard-view-css' => array( 'uri' => '/res/61ecd7cf/rsrc/css/layout/phabricator-pinboard-view.css', @@ -3859,7 +3844,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/c4fbf0c2/core.pkg.css', 'type' => 'css', ), - 'def4c982' => + 'd95b69e5' => array( 'name' => 'core.pkg.js', 'symbols' => @@ -3873,34 +3858,33 @@ celerity_register_resource_map(array( 6 => 'javelin-behavior-refresh-csrf', 7 => 'javelin-behavior-phabricator-watch-anchor', 8 => 'javelin-behavior-phabricator-autofocus', - 9 => 'phabricator-paste-file-upload', - 10 => 'phabricator-menu-item', - 11 => 'phabricator-dropdown-menu', - 12 => 'javelin-behavior-phabricator-oncopy', - 13 => 'phabricator-tooltip', - 14 => 'javelin-behavior-phabricator-tooltips', - 15 => 'phabricator-prefab', - 16 => 'javelin-behavior-device', - 17 => 'javelin-behavior-toggle-class', - 18 => 'javelin-behavior-lightbox-attachments', - 19 => 'phabricator-busy', - 20 => 'javelin-aphlict', - 21 => 'phabricator-notification', - 22 => 'javelin-behavior-aphlict-listen', - 23 => 'javelin-behavior-phabricator-search-typeahead', - 24 => 'javelin-behavior-konami', - 25 => 'javelin-behavior-aphlict-dropdown', - 26 => 'javelin-behavior-history-install', - 27 => 'javelin-behavior-phabricator-gesture', - 28 => 'javelin-behavior-phabricator-active-nav', - 29 => 'javelin-behavior-phabricator-nav', - 30 => 'javelin-behavior-phabricator-remarkup-assist', - 31 => 'phabricator-textareautils', - 32 => 'phabricator-file-upload', - 33 => 'javelin-behavior-global-drag-and-drop', - 34 => 'javelin-behavior-phabricator-reveal-content', + 9 => 'phabricator-menu-item', + 10 => 'phabricator-dropdown-menu', + 11 => 'javelin-behavior-phabricator-oncopy', + 12 => 'phabricator-tooltip', + 13 => 'javelin-behavior-phabricator-tooltips', + 14 => 'phabricator-prefab', + 15 => 'javelin-behavior-device', + 16 => 'javelin-behavior-toggle-class', + 17 => 'javelin-behavior-lightbox-attachments', + 18 => 'phabricator-busy', + 19 => 'javelin-aphlict', + 20 => 'phabricator-notification', + 21 => 'javelin-behavior-aphlict-listen', + 22 => 'javelin-behavior-phabricator-search-typeahead', + 23 => 'javelin-behavior-konami', + 24 => 'javelin-behavior-aphlict-dropdown', + 25 => 'javelin-behavior-history-install', + 26 => 'javelin-behavior-phabricator-gesture', + 27 => 'javelin-behavior-phabricator-active-nav', + 28 => 'javelin-behavior-phabricator-nav', + 29 => 'javelin-behavior-phabricator-remarkup-assist', + 30 => 'phabricator-textareautils', + 31 => 'phabricator-file-upload', + 32 => 'javelin-behavior-global-drag-and-drop', + 33 => 'javelin-behavior-phabricator-reveal-content', ), - 'uri' => '/res/pkg/def4c982/core.pkg.js', + 'uri' => '/res/pkg/d95b69e5/core.pkg.js', 'type' => 'js', ), 'dca4a03d' => @@ -3936,7 +3920,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/8aaacd1b/differential.pkg.css', 'type' => 'css', ), - '68d5f4ba' => + 'e96b08f8' => array( 'name' => 'differential.pkg.js', 'symbols' => @@ -3962,7 +3946,7 @@ celerity_register_resource_map(array( 18 => 'javelin-behavior-differential-toggle-files', 19 => 'javelin-behavior-differential-user-select', ), - 'uri' => '/res/pkg/68d5f4ba/differential.pkg.js', + 'uri' => '/res/pkg/e96b08f8/differential.pkg.js', 'type' => 'js', ), 'c8ce2d88' => @@ -4059,7 +4043,7 @@ celerity_register_resource_map(array( 'aphront-typeahead-control-css' => 'c4fbf0c2', 'differential-changeset-view-css' => '8aaacd1b', 'differential-core-view-css' => '8aaacd1b', - 'differential-inline-comment-editor' => '68d5f4ba', + 'differential-inline-comment-editor' => 'e96b08f8', 'differential-local-commits-view-css' => '8aaacd1b', 'differential-results-table-css' => '8aaacd1b', 'differential-revision-add-comment-css' => '8aaacd1b', @@ -4072,58 +4056,58 @@ celerity_register_resource_map(array( 'diffusion-icons-css' => 'c8ce2d88', 'global-drag-and-drop-css' => 'c4fbf0c2', 'inline-comment-summary-css' => '8aaacd1b', - 'javelin-aphlict' => 'def4c982', + 'javelin-aphlict' => 'd95b69e5', 'javelin-behavior' => 'fe22443b', - 'javelin-behavior-aphlict-dropdown' => 'def4c982', - 'javelin-behavior-aphlict-listen' => 'def4c982', - 'javelin-behavior-aphront-basic-tokenizer' => 'def4c982', - 'javelin-behavior-aphront-drag-and-drop' => '68d5f4ba', - 'javelin-behavior-aphront-drag-and-drop-textarea' => '68d5f4ba', - 'javelin-behavior-aphront-form-disable-on-submit' => 'def4c982', + 'javelin-behavior-aphlict-dropdown' => 'd95b69e5', + 'javelin-behavior-aphlict-listen' => 'd95b69e5', + 'javelin-behavior-aphront-basic-tokenizer' => 'd95b69e5', + 'javelin-behavior-aphront-drag-and-drop' => 'e96b08f8', + 'javelin-behavior-aphront-drag-and-drop-textarea' => 'e96b08f8', + 'javelin-behavior-aphront-form-disable-on-submit' => 'd95b69e5', 'javelin-behavior-audit-preview' => 'f96657b8', 'javelin-behavior-dark-console' => 'dca4a03d', - 'javelin-behavior-device' => 'def4c982', - 'javelin-behavior-differential-accept-with-errors' => '68d5f4ba', - 'javelin-behavior-differential-add-reviewers-and-ccs' => '68d5f4ba', - 'javelin-behavior-differential-comment-jump' => '68d5f4ba', - 'javelin-behavior-differential-diff-radios' => '68d5f4ba', - 'javelin-behavior-differential-dropdown-menus' => '68d5f4ba', - 'javelin-behavior-differential-edit-inline-comments' => '68d5f4ba', - 'javelin-behavior-differential-feedback-preview' => '68d5f4ba', - 'javelin-behavior-differential-keyboard-navigation' => '68d5f4ba', - 'javelin-behavior-differential-populate' => '68d5f4ba', - 'javelin-behavior-differential-show-more' => '68d5f4ba', - 'javelin-behavior-differential-toggle-files' => '68d5f4ba', - 'javelin-behavior-differential-user-select' => '68d5f4ba', + 'javelin-behavior-device' => 'd95b69e5', + 'javelin-behavior-differential-accept-with-errors' => 'e96b08f8', + 'javelin-behavior-differential-add-reviewers-and-ccs' => 'e96b08f8', + 'javelin-behavior-differential-comment-jump' => 'e96b08f8', + 'javelin-behavior-differential-diff-radios' => 'e96b08f8', + 'javelin-behavior-differential-dropdown-menus' => 'e96b08f8', + 'javelin-behavior-differential-edit-inline-comments' => 'e96b08f8', + 'javelin-behavior-differential-feedback-preview' => 'e96b08f8', + 'javelin-behavior-differential-keyboard-navigation' => 'e96b08f8', + 'javelin-behavior-differential-populate' => 'e96b08f8', + 'javelin-behavior-differential-show-more' => 'e96b08f8', + 'javelin-behavior-differential-toggle-files' => 'e96b08f8', + 'javelin-behavior-differential-user-select' => 'e96b08f8', 'javelin-behavior-diffusion-commit-graph' => 'f96657b8', 'javelin-behavior-diffusion-pull-lastmodified' => 'f96657b8', 'javelin-behavior-error-log' => 'dca4a03d', - 'javelin-behavior-global-drag-and-drop' => 'def4c982', - 'javelin-behavior-history-install' => 'def4c982', - 'javelin-behavior-konami' => 'def4c982', - 'javelin-behavior-lightbox-attachments' => 'def4c982', - 'javelin-behavior-load-blame' => '68d5f4ba', + 'javelin-behavior-global-drag-and-drop' => 'd95b69e5', + 'javelin-behavior-history-install' => 'd95b69e5', + 'javelin-behavior-konami' => 'd95b69e5', + 'javelin-behavior-lightbox-attachments' => 'd95b69e5', + 'javelin-behavior-load-blame' => 'e96b08f8', 'javelin-behavior-maniphest-batch-selector' => 'f85eb6d8', 'javelin-behavior-maniphest-subpriority-editor' => 'f85eb6d8', 'javelin-behavior-maniphest-transaction-controls' => 'f85eb6d8', 'javelin-behavior-maniphest-transaction-expand' => 'f85eb6d8', 'javelin-behavior-maniphest-transaction-preview' => 'f85eb6d8', - 'javelin-behavior-phabricator-active-nav' => 'def4c982', - 'javelin-behavior-phabricator-autofocus' => 'def4c982', - 'javelin-behavior-phabricator-gesture' => 'def4c982', - 'javelin-behavior-phabricator-keyboard-shortcuts' => 'def4c982', - 'javelin-behavior-phabricator-nav' => 'def4c982', - 'javelin-behavior-phabricator-object-selector' => '68d5f4ba', - 'javelin-behavior-phabricator-oncopy' => 'def4c982', - 'javelin-behavior-phabricator-remarkup-assist' => 'def4c982', - 'javelin-behavior-phabricator-reveal-content' => 'def4c982', - 'javelin-behavior-phabricator-search-typeahead' => 'def4c982', - 'javelin-behavior-phabricator-tooltips' => 'def4c982', - 'javelin-behavior-phabricator-watch-anchor' => 'def4c982', - 'javelin-behavior-refresh-csrf' => 'def4c982', - 'javelin-behavior-repository-crossreference' => '68d5f4ba', - 'javelin-behavior-toggle-class' => 'def4c982', - 'javelin-behavior-workflow' => 'def4c982', + 'javelin-behavior-phabricator-active-nav' => 'd95b69e5', + 'javelin-behavior-phabricator-autofocus' => 'd95b69e5', + 'javelin-behavior-phabricator-gesture' => 'd95b69e5', + 'javelin-behavior-phabricator-keyboard-shortcuts' => 'd95b69e5', + 'javelin-behavior-phabricator-nav' => 'd95b69e5', + 'javelin-behavior-phabricator-object-selector' => 'e96b08f8', + 'javelin-behavior-phabricator-oncopy' => 'd95b69e5', + 'javelin-behavior-phabricator-remarkup-assist' => 'd95b69e5', + 'javelin-behavior-phabricator-reveal-content' => 'd95b69e5', + 'javelin-behavior-phabricator-search-typeahead' => 'd95b69e5', + 'javelin-behavior-phabricator-tooltips' => 'd95b69e5', + 'javelin-behavior-phabricator-watch-anchor' => 'd95b69e5', + 'javelin-behavior-refresh-csrf' => 'd95b69e5', + 'javelin-behavior-repository-crossreference' => 'e96b08f8', + 'javelin-behavior-toggle-class' => 'd95b69e5', + 'javelin-behavior-workflow' => 'd95b69e5', 'javelin-dom' => 'fe22443b', 'javelin-event' => 'fe22443b', 'javelin-install' => 'fe22443b', @@ -4145,39 +4129,38 @@ celerity_register_resource_map(array( 'lightbox-attachment-css' => 'c4fbf0c2', 'maniphest-task-summary-css' => '6b1fccc6', 'maniphest-transaction-detail-css' => '6b1fccc6', - 'phabricator-busy' => 'def4c982', + 'phabricator-busy' => 'd95b69e5', 'phabricator-content-source-view-css' => '8aaacd1b', 'phabricator-core-buttons-css' => 'c4fbf0c2', 'phabricator-core-css' => 'c4fbf0c2', 'phabricator-crumbs-view-css' => 'c4fbf0c2', 'phabricator-directory-css' => 'c4fbf0c2', - 'phabricator-drag-and-drop-file-upload' => '68d5f4ba', - 'phabricator-dropdown-menu' => 'def4c982', - 'phabricator-file-upload' => 'def4c982', + 'phabricator-drag-and-drop-file-upload' => 'e96b08f8', + 'phabricator-dropdown-menu' => 'd95b69e5', + 'phabricator-file-upload' => 'd95b69e5', 'phabricator-filetree-view-css' => 'c4fbf0c2', 'phabricator-flag-css' => 'c4fbf0c2', 'phabricator-form-view-css' => 'c4fbf0c2', 'phabricator-header-view-css' => 'c4fbf0c2', 'phabricator-jump-nav' => 'c4fbf0c2', - 'phabricator-keyboard-shortcut' => 'def4c982', - 'phabricator-keyboard-shortcut-manager' => 'def4c982', + 'phabricator-keyboard-shortcut' => 'd95b69e5', + 'phabricator-keyboard-shortcut-manager' => 'd95b69e5', 'phabricator-main-menu-view' => 'c4fbf0c2', - 'phabricator-menu-item' => 'def4c982', + 'phabricator-menu-item' => 'd95b69e5', 'phabricator-nav-view-css' => 'c4fbf0c2', - 'phabricator-notification' => 'def4c982', + 'phabricator-notification' => 'd95b69e5', 'phabricator-notification-css' => 'c4fbf0c2', 'phabricator-notification-menu-css' => 'c4fbf0c2', 'phabricator-object-item-list-view-css' => 'c4fbf0c2', 'phabricator-object-selector-css' => '8aaacd1b', - 'phabricator-paste-file-upload' => 'def4c982', - 'phabricator-prefab' => 'def4c982', + 'phabricator-prefab' => 'd95b69e5', 'phabricator-project-tag-css' => '6b1fccc6', 'phabricator-remarkup-css' => 'c4fbf0c2', - 'phabricator-shaped-request' => '68d5f4ba', + 'phabricator-shaped-request' => 'e96b08f8', 'phabricator-side-menu-view-css' => 'c4fbf0c2', 'phabricator-standard-page-view' => 'c4fbf0c2', - 'phabricator-textareautils' => 'def4c982', - 'phabricator-tooltip' => 'def4c982', + 'phabricator-textareautils' => 'd95b69e5', + 'phabricator-tooltip' => 'd95b69e5', 'phabricator-transaction-view-css' => 'c4fbf0c2', 'phabricator-zindex-css' => 'c4fbf0c2', 'sprite-apps-large-css' => 'c4fbf0c2', diff --git a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php index 9649c11244..db40e92e9d 100644 --- a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php +++ b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php @@ -5,7 +5,7 @@ */ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { - protected $xhprofID; + protected $profileFilePHID; public function getName() { return 'XHProf'; @@ -13,7 +13,7 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { public function getColor() { $data = $this->getData(); - if ($data['xhprofID']) { + if ($data['profileFilePHID']) { return '#ff00ff'; } return null; @@ -25,7 +25,7 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { public function generateData() { return array( - 'xhprofID' => $this->xhprofID, + 'profileFilePHID' => $this->profileFilePHID, 'profileURI' => (string)$this ->getRequestURI() ->alter('__profile__', 'page'), @@ -33,13 +33,13 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { } public function getXHProfRunID() { - return $this->xhprofID; + return $this->profileFilePHID; } public function renderPanel() { $data = $this->getData(); - $run = $data['xhprofID']; + $run = $data['profileFilePHID']; $profile_uri = $data['profileURI']; if (!DarkConsoleXHProfPluginAPI::isProfilerAvailable()) { @@ -101,9 +101,7 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { public function willShutdown() { - if (DarkConsoleXHProfPluginAPI::isProfilerStarted()) { - $this->xhprofID = DarkConsoleXHProfPluginAPI::stopProfiler(); - } + $this->profileFilePHID = DarkConsoleXHProfPluginAPI::getProfileFilePHID(); } } diff --git a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php index 3821e54b00..484d0b0c83 100644 --- a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php +++ b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php @@ -8,6 +8,8 @@ final class DarkConsoleXHProfPluginAPI { private static $profilerStarted; + private static $profilerRunning; + private static $profileFilePHID; public static function isProfilerAvailable() { return extension_loaded('xhprof'); @@ -30,7 +32,7 @@ final class DarkConsoleXHProfPluginAPI { return false; } - public static function shouldStartProfiler() { + private static function shouldStartProfiler() { if (self::isProfilerRequested()) { return true; } @@ -55,6 +57,10 @@ final class DarkConsoleXHProfPluginAPI { return self::$profilerStarted; } + private static function isProfilerRunning() { + return self::$profilerRunning; + } + public static function includeXHProfLib() { // TODO: this is incredibly stupid, but we may not have Phutil metamodule // stuff loaded yet so we can't just phutil_get_library_root() our way @@ -69,22 +75,19 @@ final class DarkConsoleXHProfPluginAPI { public static function saveProfilerSample(PhutilDeferredLog $access_log) { - - if (!self::isProfilerStarted()) { + $file_phid = self::getProfileFilePHID(); + if (!$file_phid) { return; } - $profile = DarkConsoleXHProfPluginAPI::stopProfiler(); - $profile_sample = id(new PhabricatorXHProfSample()) - ->setFilePHID($profile); - if (self::isProfilerRequested()) { $sample_rate = 0; } else { $sample_rate = PhabricatorEnv::getEnvConfig('debug.profile-rate'); } - $profile_sample + $profile_sample = id(new PhabricatorXHProfSample()) + ->setFilePHID($file_phid) ->setSampleRate($sample_rate) ->setUsTotal($access_log->getData('T')) ->setHostname($access_log->getData('h')) @@ -92,7 +95,18 @@ final class DarkConsoleXHProfPluginAPI { ->setController($access_log->getData('C')) ->setUserPHID($access_log->getData('P')); - $profile_sample->save(); + AphrontWriteGuard::allowDangerousUnguardedWrites(true); + $caught = null; + try { + $profile_sample->save(); + } catch (Exception $ex) { + $caught = $ex; + } + AphrontWriteGuard::allowDangerousUnguardedWrites(false); + + if ($caught) { + throw $caught; + } } public static function hookProfiler() { @@ -109,22 +123,29 @@ final class DarkConsoleXHProfPluginAPI { } self::startProfiler(); - self::$profilerStarted = true; } - public static function startProfiler() { + private static function startProfiler() { self::includeXHProfLib(); xhprof_enable(); + + self::$profilerStarted = true; + self::$profilerRunning = true; } - public static function stopProfiler() { - if (!self::isProfilerStarted()) { - return null; + public static function getProfileFilePHID() { + self::stopProfiler(); + return self::$profileFilePHID; + } + + private static function stopProfiler() { + if (!self::isProfilerRunning()) { + return; } $data = xhprof_disable(); $data = serialize($data); - $file_class = 'PhabricatorFile'; + self::$profilerRunning = false; // Since these happen on GET we can't do guarded writes. These also // sometimes happen after we've disposed of the write guard; in this @@ -140,7 +161,7 @@ final class DarkConsoleXHProfPluginAPI { $caught = null; try { $file = call_user_func( - array($file_class, 'newFromFileData'), + array('PhabricatorFile', 'newFromFileData'), $data, array( 'mime-type' => 'application/xhprof', @@ -158,9 +179,9 @@ final class DarkConsoleXHProfPluginAPI { if ($caught) { throw $caught; - } else { - return $file->getPHID(); } + + self::$profileFilePHID = $file->getPHID(); } } diff --git a/webroot/index.php b/webroot/index.php index ff3e15ce71..5a9479eab1 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -136,7 +136,6 @@ try { )); DarkConsoleXHProfPluginAPI::saveProfilerSample($access_log); - } catch (Exception $ex) { PhabricatorStartup::didFatal("[Exception] ".$ex->getMessage()); }