Page MenuHomePhabricator

Fix an issue with the Herald engine field value cache
ClosedPublic

Authored by epriestley on Mar 9 2016, 6:26 PM.
Tags
None
Referenced Files
F14021264: D15451.diff
Wed, Nov 6, 6:53 AM
F14018234: D15451.id37240.diff
Tue, Nov 5, 6:41 AM
F14016596: D15451.id37241.diff
Mon, Nov 4, 10:10 AM
F14016595: D15451.id37240.diff
Mon, Nov 4, 10:10 AM
F14016594: D15451.id.diff
Mon, Nov 4, 10:10 AM
F14014380: D15451.diff
Sun, Nov 3, 2:05 AM
F14003076: D15451.diff
Sat, Oct 26, 1:56 AM
F13997203: D15451.diff
Thu, Oct 24, 2:59 AM
Subscribers
None
Tokens
"Like" token, awarded by Luke081515.2.

Details

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

Diff Detail

Repository
rP Phabricator
Branch
hcache1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 11102
Build 13749: Run Core Tests
Build 13748: arc lint + arc unit

Event Timeline

epriestley retitled this revision from to Fix an issue with the Herald engine field value cache.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Mar 9 2016, 6:27 PM
This revision was automatically updated to reflect the committed changes.