Improve performance of the Highlight processor

Thank you for investigating. I got kinda the same issue, especially when I add a second entity type so you search for node and user for example.

It seems to be related with the "content access" preprocessor when I disable it the search is fast.

This issue should live in the #790418: Search API issue queue because this has been narrowed down to the highlight processor, which is part of Search API, not Search API Pages. That said, I do have a patch in the making to address this issue.

I managed to reproduce this issue on a vanilla Drupal installation by taking the following steps:

• Install drupal using standard install profile
• Install search_api_db_defaults
• Install search_api_page
• Configure the highlighting with partial matching
• Add 250 articles using drupal crn
• Index the nodes
• Perform a search for a partially matched word
• wait 7-12 seconds
• Disable highlighting
• Clear caches
• Perform a search for the same partially matched word
• wait < 1 second

While looking through the highlighting code I noticed a couple of inefficiënt code constructs and changed those to be more efficiënt. That reduced the waiting time to 1-2 seconds when highlighting with partial matching is enabled.

@weseze Can you please test the attached patch and see if it resolves your issue?

Oh wow, this looks like it makes a ton of difference. I'm going to review this properly later today but at first glance this is looking really really awesome.

Assigning this to me, in the hope that that makes me remember to do that :)

+++ b/src/Plugin/search_api/processor/Highlight.php
@@ -275,10 +272,10 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
+      // We call array_merge using call_user_func_array to prevent having to use
+      // it in a loop because it is a resource greedy construction.
+      // @see https://github.com/kalessil/phpinspectionsea/blob/master/docs/performance.md#slow-array-function-used-in-loop
+      $text = call_user_func_array('array_merge', $item);

I already aware of these kind of optimizations. But shouldn't we follow the recommendation for PHP 5.6+?

    /* PHP below 5.6 */
    $text = call_user_func_array('array_merge', $item);

    /* PHP 5.6+: more friendly to refactoring as less magic involved */
    $text = array_merge(...$item);

@mkalkbrenner,

I agree that the php 5.6+ approach would be better, but as far as I am aware search_api still supports php 5.5, because that's the minimum version supported by Drupal 8 core, which is why I went for the old solution. I'd be more than happy to change this if the maintainers agree.

I had this assigned to me, but I don't have the bandwith to actually look at this. This looks super solid though, so it get's a +1 from me.

Wow, if the performance gain of this is really as large as you say, that would of course be huge! Thanks a lot for finding this, weseze – are you maybe able to test the patch and verify the improvements for you?
Of course many thanks to legolasbo as well, nice job with the optimizations! Just two notes:

1. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -243,10 +243,7 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   -      $excerpt_fulltext_fields = array_combine($excerpt_fulltext_fields, $excerpt_fulltext_fields);
   -      foreach ($this->configuration['exclude_fields'] as $field) {
   -        unset($excerpt_fulltext_fields[$field]);
   -      }
   +      $excerpt_fulltext_fields = array_diff($excerpt_fulltext_fields, $this->configuration['exclude_fields']);
   

   Have you actually tested whether this is an improvement?
   I remember benchmarking this once and array_diff() is really incredibly slow. At least at that time, flipping the array and then doing array_diff_key() instead was faster – and the current code does something similar.
   It’s of course nice that this is more concise, so if there’s no difference we might as well still opt for that change – but I still always get a slight jolt when seeing array_diff() used, thinking, “this could be a performance problem”.

2. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -275,10 +272,10 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   +      $text = call_user_func_array('array_merge', $item);
   

   This will result in a warning if $item is empty, and it’s not obvious that it can’t be (even with a bit of research). SO, let’s just check for that to be on the safe side.

+++ b/src/Plugin/search_api/processor/Highlight.php
@@ -243,10 +243,7 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
- $excerpt_fulltext_fields = array_combine($excerpt_fulltext_fields, $excerpt_fulltext_fields);
- foreach ($this->configuration['exclude_fields'] as $field) {
- unset($excerpt_fulltext_fields[$field]);
- }
+ $excerpt_fulltext_fields = array_diff($excerpt_fulltext_fields, $this->configuration['exclude_fields']);
Have you actually tested whether this is an improvement?
I remember benchmarking this once and array_diff() is really incredibly slow. At least at that time, flipping the array and then doing array_diff_key() instead was faster – and the current code does something similar.
It’s of course nice that this is more concise, so if there’s no difference we might as well still opt for that change – but I still always get a slight jolt when seeing array_diff() used, thinking, “this could be a performance problem”.

I did not test this change specifically, but the patch as a whole. Developed and tested on a vm running php7.2, so array_diff might still be "slow" on php 5.6. That said, it probably won't matter that much, because the number of elements in both arrays being compared won't be high enough to matter (or the site in question will have far worse problems caused by the excessive number of fields in use)

Your change looks good to me and assuming that you agreed with the rest of the patch, I think this is safe to mark RTBC. That said, I think it would be best for @weseze to test if this actually solves his problem and mark it RTBC if it does.

I remember benchmarking this once and array_diff() is really incredibly slow. At least at that time, flipping the array and then doing array_diff_key() instead was faster – and the current code does something similar.

Array_diff is indeed a bit slower, even on php 7.3.x I used https://3v4l.org to test this:

<?php

$foo = range(100, 400);
$bar = range(200, 250);

for ($i=0; $i<2500; $i++) {
    array_diff($foo, $bar);
}

With array_diff - this takes 55ms/14,8mb, with array_diff_key it takes 27ms/14,6mb.

So there is a difference on that one line, but it still seems to be faster based on

Thank a lot for benchmarking, Joris! Good to know this is still a valid concern!
However, I guess Legolasbo is still right: This is called just a single time, with pretty small arrays, so there is little chance that this will ever actually matter. So, we might as well just use the more concise version for that, even though it’s misplaced as part of a performance patch.

Also agree that it would be great if weseze could test this so we can be sure of the improvements here, and that they fix the problem. (Would really be an incredible improvement, so would be great to get this committed.)

(Also, note to Legolasbo/#5: the [#NID] markdown only works for issues, you unfortunately can’t link projects or other nodes that way.)

However, I guess Legolasbo is still right: This is called just a single time, with pretty small arrays, so there is little chance that this will ever actually matter. So, we might as well just use the more concise version for that, even though it’s misplaced as part of a performance patch.

Yes - so safe to ignore my comment there.

I agree it would be great to see a comment by @weseze but it has been quite some time since he opened this issue. Benchmarking this now with the help  of @Nick_vh

In the testing I did, I couldn't find a way to see this perform better but I also did not see it slower. This gets a +1 from me based on the results by @legolasbo.

Alright, thanks for testing!
I contacted weseze and he says he’ll have to see when he has time for testing. So, I agree, let’s just commit this.
Thanks again, everyone!

I have about 90 nodes from different types and I got 300 seconds timeout or max memory size of 1258291200 bytes exhausted.
I use highlight with checked excerpt. If I deactivate the excerpt, it worked. So performance with the excerpt (indexed field) is a problem for me.
If I changed the node types from 4 to 2 its a lot faster.

@thomaswalther please open a new issue.