Drupal\Core\TypedData\TypedData::getPropertyPath() is slow

With the change in place, and small tweaks to leverage fast string concatenation in PHP 7, I've managed to shed 17-18 ms out of the method execution time.

Holy crap! Micro optimizations that actually matter.

I meant to do this.

@ndobromirov Can you specify which version of PHP 7 you profiled against?

If we want to go for performance I would think that !$prefix would be even faster, but not sure.

Also, more importantly, this is a behavior change if $prefix is '0'. Since I don't see any discussion of that, I'm not sure whether or not this can actually happen here, so moving back to needs review.

@tstoeckler FYI: I felt comfortable RTBC'ing this since the entire test suite passes.

Are we sure that the increase in speed is from the change of strlen? Or is it because of the change in string concatenation?

Because if it is the latter, that seems a whole lot less controversial.

In any case, if #9 is a valid prefix, we should probably add a test for that as well, so we can be sure that this new patch doesn't break that behavior?

Just following for now and one comment. Keep in mind that profiling adds overhead and that overhead gets bigger with a lot of calls to fast functions. I'd imagine the improvement might be a lot less if you do a performance test without xdebug. for example just call this method many times in a loop and then check the time it took with a microtime() call before + after.

xhprof (or whatever this is exactly) might have less overhead, but I know that for example blackfire can increase the exeuction times by 200-300%.

> The overhead is roughly doubling the execution time.

Yes, but my point is that the overhead is bigger for fast functions, so yes, it will still be 35% faster than before, but this specific function might be reported as 3-4 or more times slower due to the overhead and not just 50%. Plus, empty()/isset() and array access is not a regular function and I guess is not tracked at all by xhprof. So you can't just compare how much time you have less on strlen(), but instead compare the execution time of the function.

So instead, I would try to compare it without xhprof and just do a PHP script that calls strlen() a million times or so and compare the passed time.

I did find https://stackoverflow.com/questions/6955913/isset-vs-strlen-a-fast-clear..., which does report that isset() is 6x faster *but* that was in 2011. There is a comment that says that the performance difference is only 10% on PHP 7.1 with 10k iterations (interestingly, that reported the difference got bigger with more iterations).

That is contradicted by this comment on strlen: http://php.net/manual/de/function.strlen.php#120632.

Just like the comments on the stackexchange question say, the new code is harder to understand, so my point is that we should only do this if we are *sure* that there really is a measurable performance difference.

PS: xdebug was a typo in my previous comment, should have said xhprof in both cases.

@Berdir++

Alternatively, disable caching and do 1K requests with and without the patch (and report variance) with ab. IOW: do benchmarking of a repeatable use case. If the performance difference is noticeable, it should show up there too.

Ok, that means we don't need tests, we're not changing anything functional anymore. We're only doing a different kind of concatenation now.

We should probably update the Summary of this issue to indicate that we're not longer looking at strlen/empty to make this faster. So that someone reading the issue description doesn't get confused with the patch :)

So what is the real-world performance gain that's we're observing then?

Patch applies cleanly on latest head of 9.1.x - tests triggered
Patch applies cleanly on latest head of 9.0.x - tests triggered
Patch applies cleanly on latest head of 8.9.x
Patch applies cleanly on latest head of 8.8.x

This is a win, I'll take any performance gain I can get with no harm to functionality, super!

The discussion here indicates this is no longer being done for performance (since strlen() is no longer slower than isset()), and it doesn't remove the function call.

The new version of the code is more readable - is that the only improvement here?

Re #28 well according to https://blog.blackfire.io/php-7-performance-improvements-encapsed-string... the patch here will be able leverage fast string concatenation while HEAD does not. Also in case of no prefix we are no longer doing concatenation. SO I guess we can do this. It certainly is not going to hurt.

Triggerred patch against 9.5.x, 9.4.x and 9.3.x ALL GREEN

Once getPropertyPath() has a return typehint of string we can change the strlen() to !== '' which will be a saving if this is called thousands of times. There is a value in removing this from profiles as it might make other things easier to spot. I doubt the gains a big at all here. But they will be bigger than 0.

Fixing issue summary.

So I profiled this with the following script:

class test {
  protected $name = 'test';

  public function oldf($prefix) {
    return (strlen($prefix) ? $prefix . '.' : '') . $this->name;
  }

  public function newf($prefix) {
    return strlen($prefix) ? "{$prefix}.{$this->name}" : $this->name;
  }
}

$tests = [];
for ($i = 0; $i < 100000; $i++) {
  $tests[] = random_bytes(256);
}
var_dump(count($tests));
$test = new Test();
$name = 'test';
$start = microtime(TRUE);
foreach ($tests as $string) {
  $test->newf($string);
}
$end = microtime(TRUE);
print "New function: " . ($end - $start) * 1000 . "ms\n";


reset($tests);
$start = microtime(TRUE);
foreach ($tests as $string) {
  $test->oldf($string);
}
$end = microtime(TRUE);
print "Old function: " . ($end - $start) * 1000 . "ms\n";

The new function seems to come out on tops more often than not but it is a bit random.

I then profiled a version of the function like return $prefix !== '' ? "{$prefix}.{$this->name}" : $this->name; and that was always quicker.

I did the profiling on PHP 8.1 so maybe there are further improvements to string concatenation in PHP 7 or PHP 8 that make this change not important. I think we should retask this to do the no string len at all.

new patch as per comment #36

Oops on patch 37, new patch.

Triggered a couple

\MoveBlockFormTest::testMoveBlock might be a random file, I wouldn't be so sure about the other one. Layout Builder definitely does some funky stuff with entities and fields, wouldn't be urprised if that's a real issue.

@joseph.olstad: You might want to have a look at git apply -3, that will create an inline merge conflict like a git merge and not .rej and .orig files.

tests for #38 are green for 9.3.x and 9.4.x
patch is verbatim as designed by @alexpott from #36

@berdir, yes thanks for that suggestion I will try git apply -3 next time and see what it does.

Triggering for 10.1 but was this waiting on anything else? Reading the comments and seemed to leave off on the test failures. Can wait to see what 10.1 does.

No D10 failures.

Committed 91e19e5 and pushed to 10.1.x. Thanks!

One small thing that should maybe be done in this issue as a small follow-up patch: there really should be a comment there explaining why we're using the constructs we're using, and that it was specifically done for performance reasons. Otherwise we risk regressing it in the future, since we're not (AFAIK) doing regular profiling of core.

Adding tag for a followup per #50

Followup made, #3343913: Add comments explaining performance improvement in TypedData