Page MenuHomePhabricator

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

Authored by epriestley on Mar 5 2019, 10:18 PM.
Tags
None
Referenced Files
F18740535: D20250.id.diff
Thu, Oct 2, 3:56 AM
F18736137: D20250.diff
Wed, Oct 1, 4:58 AM
F18733928: D20250.id.diff
Tue, Sep 30, 10:44 PM
F18652538: D20250.id48331.diff
Sep 21 2025, 8:29 AM
F18569470: D20250.id48331.diff
Sep 10 2025, 2:07 AM
F18518201: D20250.diff
Sep 5 2025, 2:51 PM
F18100373: D20250.diff
Aug 9 2025, 11:51 AM
F17895333: D20250.id.diff
Jul 29 2025, 6:12 AM
Subscribers
None

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 Passed
Unit
Tests Passed
Build Status
Buildable 22190
Build 30333: Run Core Tests
Build 30332: arc lint + arc unit

Event Timeline

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.