Page MenuHomePhabricator

XHProf will not build on PHP7
Open, Needs TriagePublic

Description

XHProf won't build on PHP7, im assuming this is due to all the extension changes in PHP.

The make command fails with a bunch of errors (and warnings) before failing and saying that there are too many errors.

See P1891 for the output I got.

Event Timeline

Mnkras created this task.Nov 17 2015, 4:37 AM
Mnkras updated the task description. (Show Details)
Mnkras added a project: XHProf.
Mnkras added a subscriber: Mnkras.
joshuaspence renamed this task from XHPROF will not build on PHP7 to XHPAST will not build on PHP7.Nov 17 2015, 7:05 AM
joshuaspence updated the task description. (Show Details)
joshuaspence edited projects, added XHPAST; removed XHProf.
joshuaspence added a subscriber: joshuaspence.

(Assuming you meant XHPAST rather than XHProf)

Oh sorry... I am dumb.

joshuaspence renamed this task from XHPAST will not build on PHP7 to XHProf will not build on PHP7.Nov 17 2015, 7:07 AM
joshuaspence updated the task description. (Show Details)
joshuaspence edited projects, added XHProf; removed XHPAST.

@epriestley, due to the fact that the Zend APIs for PHP7+ are different than those in PHP5* how should changes to XHProf be submitted so we don't break stuff?

Naively:

  • If the changes are relatively minor, add a bunch more #ifdefs? We already have some of this to deal with prior API changes, and I believe this approach is historically a standard one across other extensions.
  • If the changes are substantial, maybe we need to get more creative.

I don't think that is going to work here are some things I pulled from the extension migration page (unless we ifdef large blocks of code):

zval

  • PHPNG doesn't require any involvement of pointers to pointers to zval. Most occurrences of zval** variables and parameters have to be changed into zval*. The corresponding Z_*_PP() macros that work with such variables should be changed into Z_*_P().
  • In many places PHPNG work with zval directly (eliminating need for allocation and deallocation). In these cases corresponding zval* variable should be converted into plain zval, macros that use this variable from Z_*P() into Z_*() and corresponding creation macros from ZVAL_*(var, …) into ZVAL_*(&var, …). Be always careful about passing addresses of zval and & operator. PHPNG almost never require passing address of zval*. In some places & operator should be removed.
  • zval allocation macros ALLOC_ZVAL, ALLOC_INIT_ZVAL, MAKE_STD_ZVAL are removed. In most cases their usage indicate that zval* need to be changed into plain zval. Macro INIT_PZVAL is removed as well and its usages in most cases should be just removed.
  • The zval struct has completely changed.
  • PHPNG doesn't work with zval** anymore, so it doesn't need 'Z' specifier anymore. It must be replaced by 'z'.

Due to the type changes it would be really painful

Mnkras removed a subscriber: Mnkras.Dec 9 2015, 4:20 AM
Mnkras added a subscriber: Mnkras.

For my (and others') understanding, the discussion is limited to how to make the needed changes BC, otherwise https://github.com/RustJason/xhprof/tree/php7 would be fine? If so, might I suggest a new major version?

keep in mind that that was successfully compiled by removing lots of code https://github.com/RustJason/xhprof/commit/6c473e39fa93fb7f0acc3fae73f626e5c66dff1a and i'm not completely sure of the implications of the changes.

J5lx added a subscriber: J5lx.Jul 29 2016, 3:30 PM

For reference, xhprof will not be included in Debian Stretch due to lack of PHP7 support: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=846138

I have a build of this that technically works now (produces profiles, passes tests, does not segfault when gently poked). It's still extremely really rough so I don't know when I'll clean it up enough to push upstream.

I imagine the next release will be a major version increment which targets PHP7 only. I've removed "ignore functions", a little-used but complex feature. I may remove the NO_BUILTINS mode (it's somewhat pointless now since PHP7 seems to have changed some stuff?). I'll probably also remove sampling mode.

I'm possibly also going to remove the web UI and PHP support library, which are both fairly questionable, and slim the part of XHProf we actually maintain down to just the extension.

20after4 added a subscriber: 20after4.

For general context:

My PHP7 build is available in the experimental branch. All bets are off, but I've run it successfully locally since February. This may not mean much since it's one machine and one codebase (Phabricator) which uses no PHP7 features.

In fbe673127fec7d94b2ea246ea1fe585d76fd1b1f, I removed support for ignored_function_names. My recollection (which may be incorrect) was that this feature was added to reduce the overhead associated with profiling utility functions (like id() and idx()) at Facebook so that profiles were slightly more accurate. I'm not totally sure this was useful even at Facebook. The engineers working on XHProf at Facebook were generally not the engineers actually improving PHP application performance (I was in the latter group) and I think this change might have been motivated more by conceptual purity than by practical impact. I don't recall finding profiler precision at the edges to ever be the limiting factor in my ability to improve application performance, at the least.

Generally, I'd ideally like to make other similar changes to reduce the size and complexity of the XHProf extension where there isn't a clear motivation for feature inclusion. Phabricator came out of Facebook with a lot of "Facebook features" and I suspect some are still lurking in XHProf.

In adb14381b6dd880a0dd4b5020e6a731779854dfb, I removed support for older versions of PHP, and experimental will build and run only on PHP7+. I'd imagine making a major version split (PHP <7 vs PHP7+) to make things easier to maintain going forward and to also split on the feature changes. Particularly with the PHP7 changes, many of the #if blocks were just three totally different implementations, not minor API changes across versions.

I'd like to get rid of xhprof_lib and xhprof_html and provide a small utility class for managing XHProf profiles (serializing, deserializing, accessing the structure) instead. I'd like to simplify the provided UI (e.g., not bundle a copy of jQuery) and possibly split it out into a separate project ("XHProf Standalone UI") under the assumption that it's basically unsuitable for any organization doing serious performance work since, e.g., you can't share profiles with other engineers. I'd imagine generalizing the XHProf UI in Phabricator instead, and basically suggesting that you either: use the standalone UI if you're a hobbyist/trying things out; use the Phabricator UI if you use Phabricator; or build some variant of the standalone UI into your other tooling if you don't want to touch Phabricator but do plan to do real work and want to be able to share and reference profiles. To this end, the standalone UI would be refactored to aim at providing a simpler example of how to build a reasonable UI.

To experience the glory of Facebook engineering circa 2007, please reference the function xhprof_render_link(). This is a simplified, improved version of Facebook's render_link() function, which had thousands and thousands of callsites when I joined. Beyond simplifying the UI stuff, I'd like to remove all the globals and XSS and basically make the code fairly modern and reasonable.

In discussions in November 2015 with Remi Collet, I was informed that requiring a CLA should have blocked XHProf's acceptance into PECL in the first place ("signing CLA is quite blocking to be accepted as a PECL project in the first place"). I don't plan to drop the CLA requirement (see https://secure.phabricator.com/book/phabcontrib/article/cla/ for some discussion, the very brief version is that I hate contributors) and don't have access to the PECL extension. I later suggested:

I am not supportive of a PECL release bearing the name "xhprof" which is unrelated to the upstream project. I am supportive of this alternative: deprecate the PECL extension with a notice like "XHProf is no longer available through PECL because of a licensing issue. You may be able to use extensions X, Y, or Z instead, which are forks of XHProf without licensing concerns." Here, X, Y, and Z could be existing forks or a new fork, but they should not bear the name "xhprof".

I don't think anything ever came of that since it looks like the PECL extension hasn't been touched since 2013. I'd expect there will never be another PECL release, although I'm supportive of someone forking, dropping the CLA requirement, tacitly planning to never accept any third-party patches and just mirror XHProf changes so that the lack of a CLA is immaterial, and then navigating PECL with a different name so that everyone's concerns are technically satisfied.

There's also a big mess with Composer (see T6012, the brief version is that I hate it when things are easy for users) and I don't plan to restore support. See also this goofy nonsense in NPM and T5055 for our plans to reinvent the wheel in a way that makes it very difficult for normal users to install any software at all, thus keeping them safe.

If you use this and run into issues, I'm happy to upstream things if they generally look something like D10374 + D10375, or if they just delete a lot of code that seems like it shouldn't exist. If the changes are less in these veins (and, particularly, if they're "replace jQuery with Angular + React and add LOTS of C code for managing temporary directory paths"), they'll be more difficult for us/me to prioritize responding to. XHProf is a critical part of the Phabricator toolset, but it currently works properly and our customers generally don't care if the profiler functions or not so "it works" is more or less the bar we're aiming for. We've also mostly managed to structure things so that we get paid for our time now (see Support Pacts) so you're competing against requests that keep the lights on.

(I'm also open to improvements to the XHProf UI in Phabricator if you plan to use that to store and review profiles. Many obvious features like tagging and API access which would make it more usable for handling profiles of projects other than Phabricator itself are likely very easy to implement. But I'm not sure there's tons of room for real integrations, even if you used everything else in Phabricator, and you may imagine tooling which can't realistically exist in the scope of Phabricator.)