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
Unknown Object (File)
Fri, Mar 22, 3:27 AM
Unknown Object (File)
Tue, Mar 5, 3:39 PM
Unknown Object (File)
Feb 17 2024, 2:33 AM
Unknown Object (File)
Jan 31 2024, 9:53 AM
Unknown Object (File)
Dec 27 2023, 1:10 PM
Unknown Object (File)
Dec 27 2023, 5:33 AM
Unknown Object (File)
Dec 22 2023, 1:48 AM
Unknown Object (File)
Nov 30 2023, 3:48 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.