Page MenuHomePhabricator

In "qsprintf()", don't render the unmasked query string unless it will differ from the masked string
AbandonedPublic

Authored by epriestley on Tue, Mar 5, 10:18 PM.

Details

Reviewers
amckinley
Summary

See D20249. Previously, see D20067. Currently, we render each query twice: one "masked" version (with passwords replaced with "****") and one "unmasked" version. In theory, we can improve performance here by only rendering once about 99% of the time, since very few queries have passwords/keys/session tokens / etc in them.

In practice, it's a bit tricky to figure out if the masked and unmasked versions will differ or not. This appears to be the most reasonable way to do it quickly. I set things up so that if we get it wrong we fail relatively safe (executing the masked query, which will have a syntax error and fail, instead of rendering the unmasked query).

Test Plan
  • Locally, XHProf shows 20ms -> 12ms in PhutilQueryString::__construct() on a random revision page.
  • Call count for xsprintf_query() falls from ~3,800 to ~2,000.

I'll try to gather a bit more evidence for this since I'm not totally confident in XHProf for this kind of micro-optimization, as it tends to overestimate the cost of function calls.

Diff Detail

Repository
rPHU libphutil
Branch
cache2
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 22190
Build 30333: Run Core Tests
Build 30332: arc lint + arc unit

Event Timeline

epriestley created this revision.Tue, Mar 5, 10:18 PM
epriestley requested review of this revision.Tue, Mar 5, 10:18 PM
epriestley abandoned this revision.Tue, Mar 5, 10:38 PM

I can't get anything remotely convincing out of ab. This or some similar change might still be a good idea, but I'll wait until I have a clearer case for it.