We're using lovely Linkchecker in a large project and just had to run a batch update on many entities. We were wondering why it became extremely slow and saw MANY Linkchecker queries causing this.
I tried #3313343: Add option to disable cron activity, but then had to find out that it's not a cron thing, but Linkchecker also uses hook_entity_* a lot:
/**
* Implements hook_cron().
*/
function linkchecker_cron() {
if (\Drupal::config('linkchecker.settings')->get('check.disable_cron')) {
return;
}
\Drupal::service('linkchecker.extractor_batch')->processEntities();
\Drupal::service('linkchecker.checker')->queueLinks();
}
/**
* Implements hook_entity_insert().
*/
function linkchecker_entity_insert(EntityInterface $entity) {
if ($entity instanceof LinkCheckerLinkInterface) {
return;
}
if ($entity instanceof FieldableEntityInterface && !$entity->isNew()) {
/** @var \Drupal\linkchecker\LinkExtractorService $extractor */
$extractor = \Drupal::service('linkchecker.extractor');
$links = $extractor->extractFromEntity($entity);
if (!empty($links)) {
$extractor->saveLinkMultiple($links);
$extractor->updateEntityExtractIndex($entity);
}
\Drupal::service('linkchecker.clean_up')->cleanUpForEntity($entity);
}
}
/**
* Implements hook_entity_update().
*/
function linkchecker_entity_update(EntityInterface $entity) {
if ($entity instanceof LinkCheckerLinkInterface) {
return;
}
if ($entity instanceof FieldableEntityInterface) {
/** @var \Drupal\linkchecker\LinkExtractorService $extractor */
$extractor = \Drupal::service('linkchecker.extractor');
$links = $extractor->extractFromEntity($entity);
if (!empty($links)) {
$extractor->saveLinkMultiple($links);
$extractor->updateEntityExtractIndex($entity);
}
\Drupal::service('linkchecker.clean_up')->cleanUpForEntity($entity);
}
}
/**
* Implements hook_entity_delete().
*/
function linkchecker_entity_delete(EntityInterface $entity) {
if ($entity instanceof LinkCheckerLinkInterface) {
\Drupal::service('linkchecker.clean_up')->cleanUpQueues($entity->id());
return;
}
if ($entity instanceof FieldableEntityInterface) {
\Drupal::service('linkchecker.clean_up')->cleanUpForEntity($entity);
}
}
As Drupal 8+ has no "disable module" functionality, there's no way to disable it. Uninstalling removes configuration and all LinkChecker entities.
So it would be great to have a simple setting to disable linkchecker (for a while)!
Disabling link checker on cron run should still be a separate setting (#3313343: Add option to disable cron activity) as the use-cases for both are different.
As a workaround I just commented out these hooks now, but that's not a nice user-facing solution. After doing that, the batch ran fast again!
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
anybodyComment #3
anybodyJust came across this again and thought maybe we should think about the performance implications of the hook_entity_insert / *_update / *_delete again.
Our case showed that may have heavy performance implications on (mass) enttiy CRUD. So I think being able to disable them is crucial, but maybe there's even room for further improvements (in a follow-up)?
Comment #4
anybodyComment #5
tomefa commentedYes i agree also that the actual hook_entity_insert / *_update / *_delete are slowing down the website.
In some case with many link, it take more than 1 minute to save a content because of all the link extraction.
I could imagine a queue worker that will be filled with the insert/update entity id and extract the links in it later.
It doesn't need to be done right away.
For the delete action, i'm not sure.
We have patch just a bit the insert/update hook to limit to extraction to only node and media entities. Specific for our case, that already fix some performance and DB query issues because the it was run on entity that don't have any active linkchecker field (flag, group, user, etc...).
For example on a galera cluster, there were db locks issues because linkchecker was trying to check the flag entity even if there is no field that need to be checked.
Comment #6
falco010+1 faced massive performance issues due to this module entity insert/update hooks (Some nodes can have a lot of links).
For now we created an internal patch which disable both hooks (would not advice this, but for us this is very much needed at this point). This was extra problematic for us as we have all our traffic going through CDN which has a strict timeout and therefore even caused 504 errors.
Comment #7
anybodyMaybe someone could prepare a MR with such a setting?
Is a boolean option the best solution or are there other alternatives we should think about here? I like the idea of
in #5
So are we talking about modes here and how would this be self-explaining in the admin form? (UX)
Comment #10
scotwith1tOpened !156 with a first pass at this.
Answering the UX question in #7: went with a simple boolean checkbox ("Disable link extraction on every entity save") on the existing settings form, backed by state rather than config, since (in my mind, anyway) this is meant to be an operational, per-environment switch someone flips on for the duration of a specific run (a migration, a bulk import) rather than a deployable site setting -- storing it in config risked it accidentally traveling into an export/deploy from whichever environment it was last toggled on.
Only insert/update are gated; delete still runs its cleanup so removed entities don't leave orphaned tracked links behind.
Also opened #3614738 / !155 as a related, independent fix: even without this toggle, saveLink()/cleanUpForEntity() were doing roughly 2N individual queries per save for an entity with N links, which compounds the problem reported here. Worth a look together, but neither depends on the other.
Comment #11
c-logemannThanks @scotwith1t for adding code we can review and discuss. This feature request sounds very needed. I hope we can fix this soon.
Comment #12
codebymikey commentedI think this functionality can be implemented similarly to how Geocoder handled it in #3301512: Extend the ability to skip geocoding when processing a large number of entity updates, like migrations or workspace publishing
We can have a configuration/state as well as a request-level
linkchecker_disabledattribute that's checked before doing any of those processing.That way, those needing to do a batch run e.g. migrations/custom batch updates can temporarily disable it for the current request using event subscribers without touching their configs (which might be useful to still have legitimately enabled).
The MR also has merge conflicts.
Comment #13
claudiu.cristeaMaybe with #3622264: Avoid expensive operations while client waits for response there will be some relief? Could you test?
Comment #14
scotwith1t@claudiu.cristea: I tested #3622264 against the case this issue is about, bulk saves from a Drush process (a migration import, a mass update script). It gives no relief there, and it makes the memory side worse.
Drush calls kernel terminate once, when the command ends. Every fieldable entity saved during the run is queued in
LinkExtractorService::$entitiesandLinkCleanUp::$entityStack, and the extraction runs after the last save. Resetting the entity memory cache in the script does not free them; the two queues hold their own references.Measured on a site with 332k rows in
linkchecker_link: 200 published nodes with 5 links each, saved in onedrush php:scriptrun, entity caches reset every 50 saves, the site's other per-save side effects paused.So the command looks finished after 2.4 s and the process then runs for another four and a half minutes (each per-link existence query is a full-table scan on this site, 80 to 100 ms, see #3607810). Memory grows by roughly 200 KB per saved node with nothing able to reclaim it. A 30k-node import would hold roughly 6 GB at that rate; with any normal memory_limit the process dies first, and everything still queued is lost. Entities that already had an index row are then not revisited by cron, because
LinkExtractorBatch::processEntities()only selects entities with no row inlinkchecker_index.Two smaller things in the merged code, which I can open as a separate issue if you prefer: the queues also collect entities that have no scannable fields at all (content_moderation_state here), and they are keyed by entity id alone, so an entity of another type with the same id as a queued node replaces it.
array_shift()indestruct()renumbers the remaining integer keys too, so alinkcheckerlinksaved duringdestruct()can land on a still-queued entity.MR !156 is rerolled on current 2.1.x (c547417):
linkchecker.disable_entity_hooksgateshook_entity_insert()andhook_entity_update()before anything is queued.hook_entity_delete()is untouched, so removed entities still clean up.linkchecker_disable_entity_hooksdoes the same for the current request only, so a batch or an event subscriber can skip extraction without changing stored state. Same shape asgeocoder_presave_disabled.The checkbox description says that entities saved while the switch is on are not re-extracted afterwards and need a reanalysis; that is the trade the flag makes.
Comment #15
scotwith1tAlso wanted to echo these thoughts from #3622264: Avoid expensive operations while client waits for response
Super useful module, even critical from our client's standpoint. So thanks again!
Comment #16
anybody@scotwith1t thanks for pushing this forward. Totally agree with your points, thanks!
Technically, the implementation looks good to me. Maybe a native speaker could improve the description even further to make it clear in which situations this should be used. Or we could turn it around and enable it by default.
Maintainer should decide.
Comment #17
claudiu.cristeaWell, your findings are worrying me when cron is still enabled. Probably, in batch, we should call destruct() more often, maybe after each entity so we don't collect entities with Drush. That definitely need a different issue
Comment #18
claudiu.cristeaOpened #3626994: Queued entities of different types with the same ID overwrite each other
Comment #19
claudiu.cristeaOpened #3627255: Memory issues when scanning for links for the memory issue
Comment #20
claudiu.cristea@scotwith1t, it would help a lot to move on if you can run the benchmark from #14 against #3627255: Memory issues when scanning for links