Page MenuHomePhabricator
Differential D19666

Improve Herald Rule execution speed for Personal Herald rules by avoiding expensive lookup for all adapters and instead use specific adapter check
Needs ReviewPublic
Actions

Authored by artms on Sep 12 2018, 11:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 9:58 PM
Unknown Object (File)
Sun, Dec 15, 7:34 AM
Unknown Object (File)
Sat, Dec 14, 5:39 PM
Unknown Object (File)
Tue, Dec 10, 1:33 AM
Unknown Object (File)
Wed, Dec 4, 7:49 PM
Unknown Object (File)
Wed, Dec 4, 1:48 PM
Unknown Object (File)
Wed, Dec 4, 1:48 PM
Unknown Object (File)
Wed, Dec 4, 1:24 PM
View All Files
Subscribers
Korvin
Tokens
"Yellow Medal" token, awarded by Pawka.

Details

Reviewers
Pawka
epriestley
Group Reviewers
Blessed Reviewers
Summary

Adapter permission lookup is quite expensive, here we avoid looking up permissions for all adapters and instead search only one we need.

Test Plan

On empty phabricator instance (couple of users, projects) - this reduces *each* personal herald rule execution from 1.5ms to 0.3ms for Maniphest tasks.

For our production case it was reducing runtime from 20ms to 2-3ms for each Personal herald rule... We have large user base and some large projects.

This also reduces runtime for Personal Herald rules touching other objects, like differential...

Diff Detail

Repository
rP Phabricator
Branch
arcpatch-D19666
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 24968
Build 34453: Run Core Tests
Build 34452: arc lint + arc unit

Event Timeline

artms created this revision.Sep 12 2018, 11:15 AM
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptSep 12 2018, 11:15 AM
Harbormaster completed remote builds in B20799: Diff 46989.Sep 12 2018, 11:16 AM
artms requested review of this revision.Sep 12 2018, 11:16 AM
Herald added a subscriber: Korvin. · View Herald TranscriptSep 12 2018, 11:16 AM
artms marked an inline comment as done.Sep 12 2018, 11:19 AM
artms added inline comments.
src/applications/herald/adapter/HeraldAdapter.php
926

I think this is equivalent to getEnabledAdapterMap(PhabricatorUser $viewer) but without querying all adapters, hope so...

artms edited the test plan for this revision. (Show Details)Sep 12 2018, 11:20 AM
Pawka awarded a token.Sep 12 2018, 12:45 PM
artms added a comment.Sep 18 2018, 7:55 AM

Anything? This improves herald rule performance...

epriestley added a comment.Sep 18 2018, 2:27 PM

See T13196 -- anything which isn't coming through a support pact may take a very long time to get to, since you're behind all open requests from paying customers.

artms added a comment.Sep 19 2018, 5:22 PM

Uh, sorry, didn't mean to step on toes, take your time :) Just wanted to share/upstream our findings which improve overall performance for large phab installations

epriestley mentioned this in D19806: Add a `bin/herald test ...` for doing test runs via the CLI.Nov 14 2018, 11:29 PM
epriestley mentioned this in rP533e4e13b35a: Add a `bin/herald test ...` for doing test runs via the CLI.Nov 15 2018, 11:49 PM
artms updated this revision to Diff 51154.Nov 6 2020, 7:33 PM

Rebase

Harbormaster completed remote builds in B24968: Diff 51154.Nov 6 2020, 7:35 PM
artms added a comment.EditedNov 6 2020, 7:38 PM

On my personal resource limited RasberryPI alike server (4x1.2ghz ARM A53, 2GB ram, php 7.4.12) running Phabricator - this changes saves ~15ms of runtime (180ms vs 167ms after) when running:

./bin/herald test --object T19 --type HeraldManiphestTaskAdapter

It is pretty empty instance - 19 tasks, 1 user and 1 personal Herald rule...

Revision Contents
Changeset List

PathSize
src/
applications/
herald/
adapter/
5 lines
engine/
3 lines

Diff 51154

View Options

src/applications/herald/adapter/HeraldAdapter.php

Loading...
View Options

src/applications/herald/engine/HeraldEngine.php

Loading...
Phabricator · Built by Phacility