@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix an issue with the Herald engine field value cache

Summary:
To improve the performance of Herald, we attempt to generate the value for each field (e.g., a task title) only once.

For most field values this is cheap, but for some (like a commit's branches) it can be quite expensive. We only want to pay this cost once, so we cache field values.

However, D12957 accidentally added a check where we bypass the cache and generate the value for every field, before reading the cache. This causes us to generate each field for every rule that uses it, plus one extra time.

Instead, use the cache for this check, too. Also allow the cache to cache `null`, since it can be expensive to generate `null` even though the value isn't too interesting.

The value of this early hit isn't even used (we only care if it throws or not).

Test Plan:
- Wrote a rule like "if any condition matches: branches contain a, branches contain b, branches contain c".
- Put `phlog(new Exception())` in `DiffusionCommitBranchesHeraldField`.
- Before patch, saw `bin/repository reparse --herald <any commit>` compute branches three times.
- After patch, saw only one computation.
- Verified field values in the transcript view

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15451

+4 -7
+4 -7
src/applications/herald/engine/HeraldEngine.php
··· 274 274 } else { 275 275 foreach ($conditions as $condition) { 276 276 try { 277 - $object->getHeraldField($condition->getFieldName()); 277 + $this->getConditionObjectValue($condition, $object); 278 278 } catch (Exception $ex) { 279 279 $reason = pht( 280 280 'Field "%s" does not exist!', ··· 366 366 } 367 367 368 368 public function getObjectFieldValue($field) { 369 - if (isset($this->fieldCache[$field])) { 370 - return $this->fieldCache[$field]; 369 + if (!array_key_exists($field, $this->fieldCache)) { 370 + $this->fieldCache[$field] = $this->object->getHeraldField($field); 371 371 } 372 372 373 - $result = $this->object->getHeraldField($field); 374 - 375 - $this->fieldCache[$field] = $result; 376 - return $result; 373 + return $this->fieldCache[$field]; 377 374 } 378 375 379 376 protected function getRuleEffects(