From 7e37eb48273eef87e6e6811703fb5d85a3e07a81 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 16 Dec 2012 16:33:42 -0800 Subject: [PATCH] Provide a highlighter cache for Paste Summary: D4188 adds a preview of Paste contents to the list, but gets slow for large lists if you have Pygments installed since it has to spend ~200ms/item highlighting them. Instead, cache the highlighted output. - Adds a Paste highlighting cache. This uses RemarkupCache because it has automatic GC and we don't have a more generalized cache for the moment. - Uses the Paste cache on paste list and paste detail. - Adds a little padding to the summary. - Adds "..." if there's more content. - Adds line count and language, if available. These are hidden on Mobile. - Nothing actually uses needRawContent() but I left it there since it doesn't hurt anything and is used internally (I thought the detail view did, but it uses the file content directly, and must for security reasons). Test Plan: {F27710} - Profiled paste list, saw good performance and few service calls. - Viewed paste. - Viewed raw paste content. Reviewers: codeblock, btrahan, chad Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D4204 --- src/__phutil_library_map__.php | 4 +- .../controller/PhabricatorPasteController.php | 22 +--- .../PhabricatorPasteListController.php | 34 +++--- .../PhabricatorPasteViewController.php | 3 +- .../paste/query/PhabricatorPasteQuery.php | 102 +++++++++++++++--- .../paste/storage/PhabricatorPaste.php | 13 +++ .../PhabricatorBaseEnglishTranslation.php | 5 + src/view/layout/PhabricatorSourceCodeView.php | 31 +++++- .../layout/phabricator-source-code-view.css | 4 + 9 files changed, 165 insertions(+), 53 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5c3671d9d9..a2c5a03c02 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -781,7 +781,6 @@ phutil_register_library_map(array( 'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php', 'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php', 'PhabricatorFileShortcutController' => 'applications/files/controller/PhabricatorFileShortcutController.php', - 'PhabricatorFileSideNavView' => 'applications/files/view/PhabricatorFileSideNavView.php', 'PhabricatorFileStorageBlob' => 'applications/files/storage/PhabricatorFileStorageBlob.php', 'PhabricatorFileStorageConfigurationException' => 'applications/files/exception/PhabricatorFileStorageConfigurationException.php', 'PhabricatorFileStorageEngine' => 'applications/files/engine/PhabricatorFileStorageEngine.php', @@ -2066,7 +2065,6 @@ phutil_register_library_map(array( 'PhabricatorFileListController' => 'PhabricatorFileController', 'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorFileShortcutController' => 'PhabricatorFileController', - 'PhabricatorFileSideNavView' => 'AphrontView', 'PhabricatorFileStorageBlob' => 'PhabricatorFileDAO', 'PhabricatorFileStorageConfigurationException' => 'Exception', 'PhabricatorFileTransformController' => 'PhabricatorFileController', @@ -2278,7 +2276,7 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleCountdown' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleDifferential' => 'PhabricatorRemarkupRuleObjectName', 'PhabricatorRemarkupRuleDifferentialHandle' => 'PhabricatorRemarkupRuleObjectHandle', - 'PhabricatorRemarkupRuleDiffusion' => 'PhutilRemarkupRule', + 'PhabricatorRemarkupRuleDiffusion' => 'PhabricatorRemarkupRuleObjectName', 'PhabricatorRemarkupRuleEmbedFile' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleImageMacro' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleManiphest' => 'PhabricatorRemarkupRuleObjectName', diff --git a/src/applications/paste/controller/PhabricatorPasteController.php b/src/applications/paste/controller/PhabricatorPasteController.php index da6624078f..6cc57c347e 100644 --- a/src/applications/paste/controller/PhabricatorPasteController.php +++ b/src/applications/paste/controller/PhabricatorPasteController.php @@ -41,29 +41,13 @@ abstract class PhabricatorPasteController extends PhabricatorController { public function buildSourceCodeView( PhabricatorPaste $paste, - PhabricatorFile $file, $max_lines = null) { - $language = $paste->getLanguage(); - $source = $file->loadFileData(); - - if (empty($language)) { - $source = PhabricatorSyntaxHighlighter::highlightWithFilename( - $paste->getTitle(), - $source); - } else { - $source = PhabricatorSyntaxHighlighter::highlightWithLanguage( - $language, - $source); - } - - $lines = explode("\n", $source); - - if ($max_lines) { - $lines = array_slice($lines, 0, $max_lines); - } + $lines = explode("\n", rtrim($paste->getContent())); return id(new PhabricatorSourceCodeView()) + ->setLimit($max_lines) ->setLines($lines); } + } diff --git a/src/applications/paste/controller/PhabricatorPasteListController.php b/src/applications/paste/controller/PhabricatorPasteListController.php index 84167245a4..45fb6b6fe3 100644 --- a/src/applications/paste/controller/PhabricatorPasteListController.php +++ b/src/applications/paste/controller/PhabricatorPasteListController.php @@ -16,8 +16,9 @@ final class PhabricatorPasteListController extends PhabricatorPasteController { $request = $this->getRequest(); $user = $request->getUser(); - $query = new PhabricatorPasteQuery(); - $query->setViewer($user); + $query = id(new PhabricatorPasteQuery()) + ->setViewer($user) + ->needContent(true); $nav = $this->buildSideNavView($this->filter); $filter = $nav->getSelectedFilter(); @@ -76,32 +77,37 @@ final class PhabricatorPasteListController extends PhabricatorPasteController { $this->loadHandles(mpull($pastes, 'getAuthorPHID')); - $file_phids = mpull($pastes, 'getFilePHID'); - $files = array(); - if ($file_phids) { - $files = id(new PhabricatorFile())->loadAllWhere( - "phid IN (%Ls)", - $file_phids); - } - $files_map = mpull($files, null, 'getPHID'); + $lang_map = PhabricatorEnv::getEnvConfig('pygments.dropdown-choices'); $list = new PhabricatorObjectItemListView(); $list->setViewer($user); foreach ($pastes as $paste) { $created = phabricator_date($paste->getDateCreated(), $user); $author = $this->getHandle($paste->getAuthorPHID())->renderLink(); + $source_code = $this->buildSourceCodeView($paste, 5)->render(); + $source_code = phutil_render_tag( + 'div', + array( + 'class' => 'phabricator-source-code-summary', + ), + $source_code); - $file_phid = $paste->getFilePHID(); - $file = idx($files_map, $file_phid); - - $source_code = $this->buildSourceCodeView($paste, $file, 5)->render(); + $line_count = count(explode("\n", $paste->getContent())); $item = id(new PhabricatorObjectItemView()) ->setHeader($paste->getFullName()) ->setHref('/P'.$paste->getID()) ->setObject($paste) ->addAttribute(pht('Created %s by %s', $created, $author)) + ->addIcon('none', pht('%s Line(s)', number_format($line_count))) ->appendChild($source_code); + + $lang_name = $paste->getLanguage(); + if ($lang_name) { + $lang_name = idx($lang_map, $lang_name, $lang_name); + $item->addIcon('none', phutil_escape_html($lang_name)); + } + $list->addItem($item); } diff --git a/src/applications/paste/controller/PhabricatorPasteViewController.php b/src/applications/paste/controller/PhabricatorPasteViewController.php index 9c9537b1ef..03446213d8 100644 --- a/src/applications/paste/controller/PhabricatorPasteViewController.php +++ b/src/applications/paste/controller/PhabricatorPasteViewController.php @@ -20,6 +20,7 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { $paste = id(new PhabricatorPasteQuery()) ->setViewer($user) ->withIDs(array($this->id)) + ->needContent(true) ->executeOne(); if (!$paste) { return new Aphront404Response(); @@ -49,7 +50,7 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { $header = $this->buildHeaderView($paste); $actions = $this->buildActionView($user, $paste, $file); $properties = $this->buildPropertyView($paste, $fork_phids); - $source_code = $this->buildSourceCodeView($paste, $file); + $source_code = $this->buildSourceCodeView($paste); $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView()) ->addCrumb( diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index 6a9ff88d9b..a8efafa915 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -9,6 +9,7 @@ final class PhabricatorPasteQuery private $parentPHIDs; private $needContent; + private $needRawContent; public function withIDs(array $ids) { $this->ids = $ids; @@ -35,6 +36,11 @@ final class PhabricatorPasteQuery return $this; } + public function needRawContent($need_raw_content) { + $this->needRawContent = $need_raw_content; + return $this; + } + public function loadPage() { $table = new PhabricatorPaste(); $conn_r = $table->establishConnection('r'); @@ -49,20 +55,12 @@ final class PhabricatorPasteQuery $pastes = $table->loadAllFromArray($data); + if ($pastes && $this->needRawContent) { + $this->loadRawContent($pastes); + } + if ($pastes && $this->needContent) { - $file_phids = mpull($pastes, 'getFilePHID'); - $files = id(new PhabricatorFile())->loadAllWhere( - 'phid IN (%Ls)', - $file_phids); - $files = mpull($files, null, 'getPHID'); - foreach ($pastes as $paste) { - $file = idx($files, $paste->getFilePHID()); - if ($file) { - $paste->attachContent($file->loadFileData()); - } else { - $paste->attachContent(''); - } - } + $this->loadContent($pastes); } return $pastes; @@ -104,4 +102,82 @@ final class PhabricatorPasteQuery return $this->formatWhereClause($where); } + private function getContentCacheKey(PhabricatorPaste $paste) { + return 'P'.$paste->getID().':content/'.$paste->getLanguage(); + } + + private function loadRawContent(array $pastes) { + $file_phids = mpull($pastes, 'getFilePHID'); + $files = id(new PhabricatorFile())->loadAllWhere( + 'phid IN (%Ls)', + $file_phids); + $files = mpull($files, null, 'getPHID'); + + foreach ($pastes as $paste) { + $file = idx($files, $paste->getFilePHID()); + if ($file) { + $paste->attachRawContent($file->loadFileData()); + } else { + $paste->attachRawContent(''); + } + } + } + + private function loadContent(array $pastes) { + $keys = array(); + foreach ($pastes as $paste) { + $keys[] = $this->getContentCacheKey($paste); + } + + // TODO: Move to a more appropriate/general cache once we have one? For + // now, this gets automatic GC. + $caches = id(new PhabricatorMarkupCache())->loadAllWhere( + 'cacheKey IN (%Ls)', + $keys); + $caches = mpull($caches, null, 'getCacheKey'); + + $need_raw = array(); + foreach ($pastes as $paste) { + $key = $this->getContentCacheKey($paste); + if (isset($caches[$key])) { + $paste->attachContent($caches[$key]->getCacheData()); + } else { + $need_raw[] = $paste; + } + } + + if (!$need_raw) { + return; + } + + $this->loadRawContent($need_raw); + foreach ($need_raw as $paste) { + $content = $this->buildContent($paste); + $paste->attachContent($content); + + $guard = AphrontWriteGuard::beginScopedUnguardedWrites(); + id(new PhabricatorMarkupCache()) + ->setCacheKey($this->getContentCacheKey($paste)) + ->setCacheData($content) + ->replace(); + unset($guard); + } + } + + + private function buildContent(PhabricatorPaste $paste) { + $language = $paste->getLanguage(); + $source = $paste->getRawContent(); + + if (empty($language)) { + return PhabricatorSyntaxHighlighter::highlightWithFilename( + $paste->getTitle(), + $source); + } else { + return PhabricatorSyntaxHighlighter::highlightWithLanguage( + $language, + $source); + } + } + } diff --git a/src/applications/paste/storage/PhabricatorPaste.php b/src/applications/paste/storage/PhabricatorPaste.php index 0db9ec1596..239b964562 100644 --- a/src/applications/paste/storage/PhabricatorPaste.php +++ b/src/applications/paste/storage/PhabricatorPaste.php @@ -12,6 +12,7 @@ final class PhabricatorPaste extends PhabricatorPasteDAO protected $viewPolicy; private $content; + private $rawContent; public function getURI() { return '/P'.$this->getID(); @@ -66,4 +67,16 @@ final class PhabricatorPaste extends PhabricatorPasteDAO return $this; } + public function getRawContent() { + if ($this->rawContent === null) { + throw new Exception("Call attachRawContent() before getRawContent()!"); + } + return $this->rawContent; + } + + public function attachRawContent($raw_content) { + $this->rawContent = $raw_content; + return $this; + } + } diff --git a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php index dc0197fca3..e04ab69e84 100644 --- a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php @@ -193,6 +193,11 @@ abstract class PhabricatorBaseEnglishTranslation ), ), + '%s Line(s)' => array( + '%s Line', + '%s Lines', + ), + ); } diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index 6ecfce0bd8..46be49786e 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -3,6 +3,12 @@ final class PhabricatorSourceCodeView extends AphrontView { private $lines; + private $limit; + + public function setLimit($limit) { + $this->limit = $limit; + return $this; + } public function setLines(array $lines) { $this->lines = $lines; @@ -19,20 +25,39 @@ final class PhabricatorSourceCodeView extends AphrontView { $rows = array(); foreach ($this->lines as $line) { + $hit_limit = $this->limit && + ($line_number == $this->limit) && + (count($this->lines) != $this->limit); + + if ($hit_limit) { + $content_number = ''; + $content_line = phutil_render_tag( + 'span', + array( + 'class' => 'c', + ), + pht('...')); + } else { + $content_number = phutil_escape_html($line_number); + $content_line = "\xE2\x80\x8B".$line; + } // TODO: Provide nice links. $rows[] = ''. ''. - phutil_escape_html($line_number). + $content_number. ''. ''. - "\xE2\x80\x8B". - $line. + $content_line. ''. ''; + if ($hit_limit) { + break; + } + $line_number++; } diff --git a/webroot/rsrc/css/layout/phabricator-source-code-view.css b/webroot/rsrc/css/layout/phabricator-source-code-view.css index 68e12ba26e..b5ad8595b3 100644 --- a/webroot/rsrc/css/layout/phabricator-source-code-view.css +++ b/webroot/rsrc/css/layout/phabricator-source-code-view.css @@ -24,3 +24,7 @@ -ms-user-select: none; user-select: none; } + +.phabricator-source-code-summary { + margin-bottom: 4px; +}