Speed up item tracking for large sites

Since entity storages are pluggable, this could definitely never just replace the code. It might just, as in D7, provide an optional “shortcut” for when the default entity storage handler is used.
However, if you can get it working correctly, it really is considerably faster, enough people confirm it works for them and we have proper test coverage for both cases – yes, then it sounds like a nice improvement.

In any case, thanks for posting, and good luck!

We have been experiencing extremely slow rebuilds of the search index on our client site which is currently indexing over a millions nodes, 80k custom entities (without bundles), and 100 taxonomy terms.

We determined that a huge performance issue is present in the core Entity Query logic. It will perform a join against the node_field_data table no matter if you need that for the node query you're performing. Since the base table for nodes is the node table, this adds an inefficiency that is causing hours of delays in getting items into the search index.

Some benchmarks from my local testing:

Before my update (Search API 8.x-1.13):

• 2,800 nodes/min
• 168,000 nodes/hour
• Finished in: ~ 6hr 15 minutes for 1.04 million nodes

After my update:

• 11,900 nodes/min
• 1,428,000 nodes/hour
• Finished in: ~ 45 minutes for 1.04 million nodes

I have updated getPartialItemIds to use a database query for the selection of the nodes only. The loading I have left untouched. Please see attached patch for review.

I have also updated the original ticket description with a my proposed resolution.

Marking as Needs review to kick off tests.

Is the patch in #11 based on #8 or is it a new approach? Can we get an interdiff if it is the same approach?

Those numbers do seem to be a lot bigger than I expected, nice!

This looks like a very nice speed improvement.

I don't think this needs tests? Or do we want to test somehow that indexing is faster than before?

I don't think this needs tests? Or do we want to test somehow that indexing is faster than before?

You are right, this is not needed to be tested. We have coverage for the functionality already (for both issues).

When I opened this issue, my intention was to speed up the initial entity tracking. That is still a valid problem, so I would like to have a different issue for these new performance improvements.

Yes, let's move #11 to it's own issue.

Thanks for posting this and keeping the issue going!
We actually made related improvements just recently in #2881689: Tracker-Sync Task does not scale well with a lot of content (necessitating a re-roll here – see attached). Maybe try whether that already helps somewhat?
Anyways, the change worked on here was also suggested in that other issue, and still makes a lot of sense. However, my objection is still the same as before (see #10): Since entity storage is pluggable, we can’t just assume there will always be a base table which looks exactly the way we want it. This will have to be an optional variant, based on the existence of a base table, maybe the appropriate table columns and maybe even some hidden config setting to make sure people can also disable this if it causes problems for them. (Though the latter can probably wait until someone complains – with our test coverage, we should have been warned if this would break anything major.)

Other than that, the patch looks very good, thanks!

@ chr.fritsch: The new patch actually does tackle that same problem, the description just didn’t make this very clear. (Also leading to a wrong issue title.)
Have you tried whether it also leads to such a large speed improvement for you? As tests are not failing for that one, it seems like it’s closer to be RTBC than yours.

Still needs work, in any case, as outlined above.

Awesome patch! I was looking at a similar issue with a similar volume of nodes. We are supporting a master/slave DB set up and looking through the patch I was just wondering why this query isn't offloaded to the replica if its available? It looks like this read would be a prime candidate from the replica.

BTW. Found that Duplicated task records added into the search_api_task table as the time goes by while tracking. The more records to track, the slower, with more duplicated tasks added. It makes the great improvement here helpless

Awesome patch! I was looking at a similar issue with a similar volume of nodes. We are supporting a master/slave DB set up and looking through the patch I was just wondering why this query isn't offloaded to the replica if its available? It looks like this read would be a prime candidate from the replica.

Not sure exactly how to do that in a generic way. Can you adapt the patch accordingly?

In general, very unfortunate that no-one seems to be willing to bring this great improvement over the finish line. It really shouldn’t be that complicated.

@drunken monkey,

I installed this and am running it on a dev env that has over 300,000 items to index.

I ran drush9 sapi-c && drush9 sapi-r && drush9 sapi-i and so far so good. I will continue to test over the next week or so... but so far so good.

@drunken monkey I have applied the updates to the patch you've recommended. At least, I hope I have captured your notes.

Specifically, I added a check for a base table in the query. If the base table exists, it swaps over to a direct database query. If the base table doesn't exist, it falls back on the entity query structure that is currently in place.

Moving to Needs Review to kick off tests.

Great job, thanks a lot!

I just think you accidentally removed teh $select->accessCheck(FALSE); call for the entity query branch? Also, the $database property was missing, but other than that this seems pretty flawless.

Revised patch attached. Would be great if someone else could test/review, too, but generally this seems RTBC to me. (In theory, we’d now probably need a test entity type without base table for testing, but I really don’t think that’s worth it.)

Thanks for the review! Yeah. I didn't mean to remove the accessCheck line.

+1 for peer review. :-)

I was worried about whether this would still work for NoSQL backends, so I asked chx on Slack. He told me that the safest way would be to move the code to a service and tag it with backend_overridable, so DB drivers could easily override it, if necessary. (See the change record.)

As I still don’t know whether this wouldn’t just work out-of-the-box with NoSQL backends (as we just use a fairly trivial SELECT) I’m not sure whether this is really necessary. Maybe we can still just commit it as-is and wait for complaints. But if someone reading this uses a NoSQL backend, it would be really great to get feedback from them. I guess as long as the NoSQL backend drivers wouldn’t provide their own version of our new service, this would still break for NoSQL users after updating Search API anyways.
(While having it as a service would also make it easy for developers to override the code in other circumstances, I don’t think it would make it much easier than just overriding the plugin class.)

So, in conclusion, I have now just one more option and am still unsure what to do. Opinions very much appreciated!

As I still don’t know whether this wouldn’t just work out-of-the-box with NoSQL backends (as we just use a fairly trivial SELECT)

The D7 mongodb driver attempted SQL parsing but it was very difficult. This has changed with https://github.com/vimeo/php-mysql-engine which was released this year and provides a full MySQL compatible parser in PHP. Nonetheless, even if the parsing is done, translation is not trivial. So I believe this would not work with any NoSQL backends.

https://gitlab.com/daffie/mongodb is the current MongoDB entity storage attempt. I would recommend talking to @daffie on slack.

The D7 mongodb driver attempted SQL parsing but it was very difficult. This has changed with https://github.com/vimeo/php-mysql-engine which was released this year and provides a full MySQL compatible parser in PHP. Nonetheless, even if the parsing is done, translation is not trivial. So I believe this would not work with any NoSQL backends.

Ah, sorry, seems I didn’t explain well enough. We don’t use plain SQL in this patch, we use the DB abstraction layer (\Drupal\Core\Database\Connection::select() etc.). And from the project description, that should be fine.
So, I think I’d go with committing this as-is for now and waiting whether anyone complains regarding NoSQL. It’s really pretty easy to change this even after committing, and that way we’d also already know that someone would really implement the NoSQL compatibility code. (Having a tagged service that no-one overrides won’t help anyone, either.)

Still, one final call for people to please test whether this breaks anything for them, or whether the code looks good! I’ll commit this in a week or two.

Committed.
Thanks a lot again, everyone!