Page MenuHomePhabricator

Slightly improve base85 performance for 64-bit systems
ClosedPublic

Authored by epriestley on Apr 25 2018, 4:30 PM.
Tags
None
Referenced Files
F14394713: D19408.diff
Sun, Dec 22, 2:25 AM
Unknown Object (File)
Sun, Dec 15, 4:14 PM
Unknown Object (File)
Thu, Dec 12, 9:31 PM
Unknown Object (File)
Mon, Dec 9, 4:50 PM
Unknown Object (File)
Fri, Dec 6, 6:07 AM
Unknown Object (File)
Mon, Dec 2, 8:02 PM
Unknown Object (File)
Mon, Dec 2, 8:02 PM
Unknown Object (File)
Mon, Dec 2, 8:02 PM
Subscribers
None

Details

Summary

Ref T13130. I wasn't able make this much better, but it looks like this is about ~20% faster on my system.

This kind of thing is somewhat difficult to micro-optimize because XHProf tends to over-estimate the cost of function calls. In XHProf, this looks much much faster than the old version (~100% faster) but the actual cost of bin/conduit call --method differential.getrawdiff hasn't improved that much. Still, it seems consistently faster across multiple runs.

Test Plan
  • Pulled binary diffs over Conduit with bin/conduit call --method differential.getrawdiff.
  • Verified that they are byte-for-byte identical with the pre-change diffs, and look like they're ~20% faster.
  • Profiled the differences and saw a far more dramatic improvement, but I believe XHProf is exaggerating the effect of this change because it tends to overestimate function call cost.
  • Ran unit tests (from D19407), got byte-for-byte identical output under both 32bit and 64bit mode.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

As opposed to going too deep down this rabbit hole, maybe we should check if this extension is installed and just use that if available? AFAIK we don't depend on any other extensions that can't be installed via PECL, but at least it would help this one customer.

This revision is now accepted and ready to land.Apr 25 2018, 5:12 PM

I imagine a future world where we ship a PHP extension ourselves that includes C implementations of base85, a lot of utf8 stuff, etc., (T2312) but I'm hesitant to start adding optional PHP extension dependencies before that, at least without a stronger use case. One specific concern is that if PHP segfaults it's hard to figure out why (and I actually don't even know how to pull a backtrace out of an Apache coredump reliably), so I feel under-equipped to support new extensions. There's also a general lack of other peripheral support today, like reporting installed extensions in Config > Versions.

The next change here should also just totally moot this by making the renderer bail early way before we get to this code, which is the actual intended attack on the problem the install hit.

That particular extension also just does the inner base85 bit, not the Git encoding around it. The inner bit is more expensive so it would get most of the possible improvement, but the entire function could easily go to C for an even larger improvement.

This change and D19407 are ultimately super low-impact to the point of being kind of pointless, I just thought this was like 300x slower than it is based on assumptions from the original report and by the time I figured out it apparently isn't that slow I already more or less had a slightly faster version of it. I could kind of just throw this and D19407 out, but it feels like the test coverage is in a slightly better place now, maybe? And it's a bit faster and I think not particularly more complex.

We'd also want to make a similar structural change to the tests if we did add extension support (since we'd need to keep the PHP version around, and that would let the unit tests check that the PHP version and C version have the same behavior) so maybe that's a good argument for these changes, too.

This revision was automatically updated to reflect the committed changes.