Skip to content

Commit 5d0ae28

Browse files
author
epriestley
committedMay 20, 2020
Put a readthrough cache in front of inline context construction
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it. Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21279
1 parent d2d7e7f commit 5d0ae28

File tree

5 files changed

+123
-10
lines changed

5 files changed

+123
-10
lines changed
 

‎src/applications/differential/query/DifferentialDiffInlineCommentQuery.php

+15-6
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,26 @@ protected function loadHiddenCommentIDs(
6767
return $id_map;
6868
}
6969

70+
protected function newInlineContextFromCacheData(array $map) {
71+
return PhabricatorDiffInlineCommentContext::newFromCacheData($map);
72+
}
73+
7074
protected function newInlineContextMap(array $inlines) {
7175
$viewer = $this->getViewer();
72-
7376
$map = array();
7477

78+
$changeset_ids = mpull($inlines, 'getChangesetID');
79+
80+
$changesets = id(new DifferentialChangesetQuery())
81+
->setViewer($viewer)
82+
->withIDs($changeset_ids)
83+
->needHunks(true)
84+
->execute();
85+
$changesets = mpull($changesets, null, 'getID');
86+
7587
foreach ($inlines as $key => $inline) {
76-
$changeset = id(new DifferentialChangesetQuery())
77-
->setViewer($viewer)
78-
->withIDs(array($inline->getChangesetID()))
79-
->needHunks(true)
80-
->executeOne();
88+
$changeset = idx($changesets, $inline->getChangesetID());
89+
8190
if (!$changeset) {
8291
continue;
8392
}

‎src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php

+4
Original file line numberDiff line numberDiff line change
@@ -70,4 +70,8 @@ protected function newInlineContextMap(array $inlines) {
7070
return array();
7171
}
7272

73+
protected function newInlineContextFromCacheData(array $map) {
74+
return PhabricatorDiffInlineCommentContext::newFromCacheData($map);
75+
}
76+
7377
}

‎src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php

+20
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,26 @@ final class PhabricatorDiffInlineCommentContext
88
private $bodyLines;
99
private $tailLines;
1010

11+
public static function newFromCacheData(array $map) {
12+
$context = new self();
13+
14+
$context->setFilename(idx($map, 'filename'));
15+
$context->setHeadLines(idx($map, 'headLines'));
16+
$context->setBodyLines(idx($map, 'bodyLines'));
17+
$context->setTailLines(idx($map, 'tailLines'));
18+
19+
return $context;
20+
}
21+
22+
public function newCacheDataMap() {
23+
return array(
24+
'filename' => $this->getFilename(),
25+
'headLines' => $this->getHeadLines(),
26+
'bodyLines' => $this->getBodyLines(),
27+
'tailLines' => $this->getTailLines(),
28+
);
29+
}
30+
1131
public function setFilename($filename) {
1232
$this->filename = $filename;
1333
return $this;

‎src/infrastructure/diff/interface/PhabricatorInlineComment.php

+10
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,16 @@ public function getStorageObject() {
8282
return $this->storageObject;
8383
}
8484

85+
public function getInlineCommentCacheFragment() {
86+
$phid = $this->getPHID();
87+
88+
if ($phid === null) {
89+
return null;
90+
}
91+
92+
return sprintf('inline(%s)', $phid);
93+
}
94+
8595
abstract protected function newStorageObject();
8696
abstract public function getControllerURI();
8797

‎src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php

+74-4
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
abstract class PhabricatorDiffInlineCommentQuery
44
extends PhabricatorApplicationTransactionCommentQuery {
55

6+
const INLINE_CONTEXT_CACHE_VERSION = 1;
7+
68
private $fixedStates;
79
private $needReplyToComments;
810
private $publishedComments;
@@ -19,6 +21,7 @@ abstract protected function loadHiddenCommentIDs(
1921
array $comments);
2022

2123
abstract protected function newInlineContextMap(array $inlines);
24+
abstract protected function newInlineContextFromCacheData(array $map);
2225

2326
final public function withFixedStates(array $states) {
2427
$this->fixedStates = $states;
@@ -268,16 +271,83 @@ protected function didFilterPage(array $inlines) {
268271
}
269272

270273
if ($need_context) {
271-
$context_map = $this->newInlineContextMap($need_context);
274+
$this->loadInlineCommentContext($need_context);
275+
}
276+
}
277+
278+
return $inlines;
279+
}
280+
281+
private function loadInlineCommentContext(array $inlines) {
282+
$cache_keys = array();
283+
foreach ($inlines as $key => $inline) {
284+
$object = $inline->newInlineCommentObject();
285+
$fragment = $object->getInlineCommentCacheFragment();
286+
287+
if ($fragment === null) {
288+
continue;
289+
}
290+
291+
$cache_keys[$key] = sprintf(
292+
'%s.context(v%d)',
293+
$fragment,
294+
self::INLINE_CONTEXT_CACHE_VERSION);
295+
}
296+
297+
$cache = PhabricatorCaches::getMutableStructureCache();
298+
299+
$cache_map = $cache->getKeys($cache_keys);
300+
301+
$context_map = array();
302+
$need_construct = array();
303+
304+
foreach ($inlines as $key => $inline) {
305+
$cache_key = idx($cache_keys, $key);
272306

273-
foreach ($need_context as $key => $inline) {
274-
$inline->attachInlineContext(idx($context_map, $key));
307+
if ($cache_key !== null) {
308+
if (array_key_exists($cache_key, $cache_map)) {
309+
$cache_data = $cache_map[$cache_key];
310+
$context_map[$key] = $this->newInlineContextFromCacheData(
311+
$cache_data);
312+
continue;
275313
}
276314
}
277315

316+
$need_construct[$key] = $inline;
278317
}
279318

280-
return $inlines;
319+
if ($need_construct) {
320+
$construct_map = $this->newInlineContextMap($need_construct);
321+
322+
$write_map = array();
323+
foreach ($construct_map as $key => $context) {
324+
if ($context === null) {
325+
$cache_data = $context;
326+
} else {
327+
$cache_data = $this->newCacheDataFromInlineContext($context);
328+
}
329+
330+
$cache_key = idx($cache_keys, $key);
331+
if ($cache_key !== null) {
332+
$write_map[$cache_key] = $cache_data;
333+
}
334+
}
335+
336+
if ($write_map) {
337+
$cache->setKeys($write_map);
338+
}
339+
340+
$context_map += $construct_map;
341+
}
342+
343+
foreach ($inlines as $key => $inline) {
344+
$inline->attachInlineContext(idx($context_map, $key));
345+
}
346+
}
347+
348+
protected function newCacheDataFromInlineContext(
349+
PhabricatorInlineCommentContext $context) {
350+
return $context->newCacheDataMap();
281351
}
282352

283353
final protected function simplifyContext(array $lines, $is_head) {

0 commit comments

Comments
 (0)
Failed to load comments.