MJ Convento

/

PHP 8, three months in: the JIT was the least of it

2,159 words, about 10 min read

The last of our twelve app servers went to PHP 8.0 on 25 January, nine weeks after 8.0.0 shipped on 26 November. The JIT is off on all of them. It has been off since the second week of the canary, and switching it off was the least interesting decision we made in the entire upgrade.

The platform runs Symfony 4.4 LTS, MySQL 5.7, Redis for sessions and the rate cache, PHP-FPM on twelve EC2 instances behind a load balancer. Eighteen developers, GitLab CI. We had been on 7.4 since the end of 2019, so 8.0 was one version of catch-up plus a major release worth of semantic change. The semantic change is what cost us, and none of it was the part everybody wrote about in November.

The two changes that broke production

The first one is saner string to number comparisons. Non-strict comparison between a number and a non-numeric string no longer casts the string to a number. 0 == "foo" is false now, and so is 0 == "". Nikita’s RFC says the practical impact is lower than you would intuitively expect, and that it heavily depends on the codebase. Ours is eight years old and full of partner integrations, so we were on the wrong side of that sentence.

One of our courier partners posts shipment status webhooks. Their docs say status is an integer. For shipments created but not yet accepted they send an empty string, which is not in the docs and which nobody here had noticed for two years, because on 7.4 "" == 0 was true and the empty string fell into the first branch of a switch and was read as pending. That is exactly the right answer, arrived at by accident.

We had also rewritten that switch as a match expression during the upgrade, because match is the obvious shape for a mapper. Match compares with === regardless of strict_types, and it throws when nothing matches. So two changes stacked on one function and we got this at 13:41 on 27 January:

[2021-01-27 13:41:52] request.CRITICAL: Uncaught PHP Exception UnhandledMatchError:
"Unhandled match case ''" at /var/www/app/src/Partner/Webhook/StatusMapper.php line 38

Forty-one shipments sat in an unmapped state for about ninety minutes until the on-call engineer read the log and shipped this:

-        return match ($payload['status']) {
+        $raw = (string) ($payload['status'] ?? '');
+        if ($raw === '') {
+            return ShipmentStatus::PENDING;
+        }
+
+        return match ((int) $raw) {
             0 => ShipmentStatus::PENDING,
             1 => ShipmentStatus::PICKED_UP,
             2 => ShipmentStatus::IN_TRANSIT,
             3 => ShipmentStatus::DELIVERED,
         };

I am glad it threw. On 7.4 the same undocumented value would have carried on being silently correct until the partner changed it to "0" or null and then been silently wrong. But it threw in production, on a Wednesday afternoon, at the busiest hour of our week, and that is on us for not grepping harder for loose comparisons against literal zero. We found eleven more that week. Nine were harmless. Two were the same class of accident.

The second one is consistent type errors for internal functions, combined with saner numeric strings. Passing a bad type to a built-in function used to emit a warning and return null or false. Now it throws a TypeError, and a leading-numeric string like "2.5 kg" is no longer acceptable where a float is expected. Our weekly rate card importer reads a partner CSV in which the weight column is usually a number and occasionally has a unit glued to it:

[2021-02-03 09:12:44] app.ERROR: Uncaught PHP Exception TypeError:
"round(): Argument #1 ($num) must be of type int|float, string given"
at /var/www/app/src/Pricing/WeightBracket.php line 74
-        $kg = round($row['weight'], 2);
+        if (!is_numeric($row['weight'])) {
+            throw new InvalidRateRow(sprintf('weight "%s" is not numeric', $row['weight']));
+        }
+
+        $kg = round((float) $row['weight'], 2);

On 7.4 that line logged a notice and used 2.5, which was the number we wanted anyway. The importer now refuses the row and names it. Better behaviour, and it still broke a scheduled job in production, because we had never read the notices. Which brings up the third thing, the one that was not a bug but did wake somebody: error_reporting now defaults to E_ALL and display_startup_errors is on by default. Undefined array key reads went from notice to warning at the same time. Our first canary box wrote roughly 900 MB of log in its first hour. We pinned error_reporting and display_errors explicitly in the FPM pool config, then spent two sprints actually fixing the warnings rather than muting them. About four hundred of them. Most were real, in the sense that they described code that was guessing.

One more, if you have an old error handler: @ no longer silences fatal errors, and a handler that tests error_reporting() == 0 to detect suppression has to become a mask check, !(error_reporting() & $err_no).

Where the JIT went

We gave it a fair run. Two canary boxes with opcache.jit_buffer_size=64M and opcache.jit=1235, which is the configuration the JIT RFC itself suggests, for eleven days of production traffic against ten unmodified boxes. p95 on the booking search endpoint went from 351 ms to 349 ms. CPU on the JIT boxes was flat. Requests per second per box on our synthetic replay was inside the noise band of the ten runs we had already done. We reverted the two boxes and have not been back.

This is not a surprise if you read the RFC instead of the headlines. Zend’s own numbers in it: Mandelbrot gets more than four times faster, bench.php more than twice, PHP-Parser about 1.3 times faster according to Nikita, an amphp hello world 5 percent. And then, plainly: “it currently doesn’t seem to significantly improve real-life apps like WordPress (with opcache.jit=1235 326 req/sec vs 315 req/sec)”. Three percent on a benchmark that is closer to our workload than Mandelbrot will ever be.

A Symfony request is not CPU bound in the way the JIT rewards. Ours spends its time in MySQL, in Redis, in serialization, in the container, and in a lot of very short function calls the interpreter already handles well. The RFC’s stated motivation is openly strategic: exhausting other optimisation options, opening PHP to non-web CPU-heavy work, eventually writing built-in functions in PHP rather than C. Good reasons for the project. Not reasons for me to spend 64 MB of shared memory per box, and accept a class of bug the RFC itself says will be harder to debug, in exchange for two milliseconds.

Then there is tooling. The RFC says it directly: JIT affects third party debuggers and profilers, Xdebug and Blackfire and the rest. Our profiler is the thing that tells us where the time actually goes.

What 8.0 gave us without the JIT

Same code, same queries, same hardware, opcache on and JIT off, comparing the week before the fleet rollout to the week after:

MeasurePHP 7.4PHP 8.0
p95, booking search endpoint412 ms351 ms
median, booking create API128 ms111 ms
mean FPM worker RSS74 MB68 MB
nightly reconciliation job22 min18 min

Those are our numbers on our workload and I would not bet a cent on them transferring to yours. Roughly 13 percent off the p95 of the endpoint our couriers hit most, for the price of a version bump, is a good quarter. The reconciliation job gained more than the web path did, which fits: it is the most arithmetic-heavy thing we run.

The features that changed how we type

Constructor property promotion is the one I did not expect to care about and now use everywhere. Our value objects were four repetitions of every property. Now:

final class RateQuote
{
    public function __construct(
        public string $originZone,
        public string $destinationZone,
        public float $chargeableWeight,
        public int $amountInCentavos,
        public ?string $promoCode = null,
    ) {}
}

Note the trailing comma, which 8.0 also allows, and note that the nullable type has to be written out: a null default does not make a promoted property nullable for you. The RFC is explicit and the engine is unforgiving.

Named arguments did more for readability than I predicted, mostly at call sites we had been writing badly for years:

// before
$quote = $this->pricing->quote($origin, $destination, $weight, false, true);

// after
$quote = $this->pricing->quote(
    origin: $origin,
    destination: $destination,
    weightKg: $weight,
    insured: false,
    codEnabled: true,
);

The catch nobody warns you about: your parameter names are now part of your public API. We renamed two parameters in a shared internal package in February and broke a caller in our internal operations app. A parameter rename is a version bump now, same as anything else.

The nullsafe operator earns its place in Twig-adjacent controller code and nowhere else so far. $shipment->getConsignee()?->getAddress()?->getProvince() replaced a four-level nest. It short circuits fully, so arguments to skipped calls are never evaluated, which is the behaviour you would guess and not the behaviour every language picked. And str_contains(), str_starts_with() and str_ends_with() quietly removed about sixty strpos() !== false comparisons from our codebase. Eight years of strpos bugs waiting to happen, gone by find and replace.

The ecosystem, as of last week

Composer 2.0.0 landed on 24 October, a month before PHP 8, and it made the upgrade both easier and noisier. Easier because partial updates are much faster and resolution errors name the conflict clearly. Noisier because it writes a platform check into vendor/autoload.php that fails hard when the running PHP does not match the lock file, which caught two of our deploy scripts running the wrong binary.

Three of our dependencies still declared php: ^7.2 in early January with working 8.0 code behind it. Composer 2 added a singular --ignore-platform-req, so instead of ignoring every platform requirement we could ignore exactly one:

composer update --ignore-platform-req=php

Two of the three have since tagged releases with ^8.0. The third has not, and we are still running it with the flag in CI, which I dislike and which is documented in our README with a link to the upstream issue. PHPUnit 9.3 had already declared "php": "^7.3 || ^8.0" back in August, so the test suite was never the blocker. Symfony was not either: 4.4.17, tagged three days after PHP 8.0.0, carries a pile of PHP 8 fixes, and 4.4 only requires PHP 7.1.3. We ran a two version CI matrix on 7.4 and 8.0 for six weeks, which pushed the pipeline from eleven minutes to nineteen. Worth it. We dropped 7.4 from the matrix on 1 February.

We are staying on 4.4 for now. Symfony 5.2 is the release that lets you define routes and required dependencies with PHP 8 attributes, and I want it, but not in the same quarter as a language upgrade. So attributes are in the language and unused by us, which is a slightly silly place to be.

What we postponed

Assertion failures throw by default now instead of warning, so we left assert.exception=0 in the FPM pools until we have read every assert() in the tree. There are thirty-one. Sorting is stable in 8.0, which means a few of our comparators are returning arbitrary orders less often, and I have not yet worked out whether any list in the admin UI depended on the old instability. The @ audit is half done. And we have not touched the loose comparison problem at the root: about 1,900 uses of == remain, and a static analyser cannot tell us which ones matter, because the RFC says so itself.

We are on 8.0.3, released on 4 March, deployed last Friday. Nothing forced this upgrade: 7.4 came out on 28 November 2019 and under php.net’s two years active, two years security policy it has active support until late this year and security fixes for a year after that. We did it early because doing it early meant doing it while the 8.1 feature list was still being argued about rather than shipped. Ask me again in November whether that was the right call.

Sources