Use LRU Cache for static entity cache

Either in this issue or a follow-up, we should remove or modify MigrateExecutable::attemptMemoryReclaim()

alexpott → changed the visibility of the branch 3498154-use-lru-cache to hidden.

re: slot count:

From #1199866-84: Add an in-memory LRU cache → , the thought was that this should be a much higher number then 300. So it is nice to see a number of 1000. Do we know if smaller hosting, e.g. an entry level pantheon plan will hold 1000 entity slots for 5-6 fields on a node or paragraph?

@heddn well atm the moment the number of slots in your cache is infinite so this is not really making anything worse. 5-6 fields on node is not that much - I tried to get the amount of memory used by a node on a production site with a complex data model. I think it might in the realms of 0.2mb per node... so a 1000 of them is about 200mb which is well within what I would expect you to be running a migrate job with. I expect that most sites nodes would take up considerably less space.

alexpott → closed merge request !10845

Now that 📌 Add an in-memory LRU cache Needs work is in we should use it!

Very nice. Looks like StandardPerformanceTest needs a tweak.

No sure I see why this affects standard performance test.

Could that be a random fail? https://git.drupalcode.org/project/drupal/-/pipelines/440609/test_report... does say it failed quite a lot recently on 11.x.

Hmm I think I saw a similar failure somewhere else, maybe it's a random failure. Not sure we need those assertions now we have the grouped cache tag assertions so might be better to just drop them entirely.

Yeah, I've been meaning to file an issue about that, started leaving them out in some cases, they no have no performance meaning now that we can assert the amount of actual lookup queries.

I can't trigger a retest on the MR or I would have done that, saw a similar comment recently, apparently an issue with merge requests created by core maintainers?

The permissions on pipelines are different depending on whether they're triggered by a commit by a core committer or not, I do not really understand why but it's inherent to gitlab. Even the directory it runs in is different which was very annoying when trying to add phpstan et al result caching (it only worked for core committer MRs for a couple of weeks until we noticed).

Opened 📌 Remove cache tag checksum assertions from performance tests Active .

Test went green on re-running.

Seems pretty straight forward, not entirely sure how else to review.

#16 has also already been committed

Should we document 'entity.memory_cache.slots: 1000' and/or its usefulness for migrations somewhere? Seems like something that could save a lot of time if we explain it beforehand rather than when found during debugging of your migration's memory issues.

MigrateExecutable used to release the entities from memory but no longer does, yet we have (minor) concerns from @heddn that people (with free hosting plans) will run out of memory. To me, that seems like a behavioral change that could lead to some nasty hour-long debugging sessions figuring out why stuff isn't working anymore.

So I'm not sure about RTBC. Seems like we may have some collateral damage here if we release this without a CR or documentation.

We could set the number of slots in a compiler pass and change it if you have less memory.

Memory could be different for web vs. cli so I'm not sure about changing it dynamically like that.

I'd rather we leave it at 1k but at least mention this in the release notes, accompanied with a CR explaining you can change this behavior with a container param. Adding dynamic memory profiling is perhaps a bit overkill and may lead to obscure bugs.

I'll draft a CR, one sec.

https://www.drupal.org/node/3512407 →

Does this seem sufficient? It won't "kill" migrations per se, but it will trigger more batches and as a result may end up loading the same entities into memory over and over for every batch, leading to slower migrations.

CR reads really well to me.

Agreed with leaving it at a fixed value, the scope here is that it shouldn't affect web requests and it can be lowered if necessary.

The problem is if you are on minimal hosting changing container params is a right pfaff and knowing that you have to do that is going to be very hard to find out.

Couldn't people change this from their services.yml file which is automatically picked up because it's in the container_yamls setting?

Which reminds me we didn't add this new container param to default.services.yml. We might want to do that as it gives us the opportunity to write some brief documentation in its comment.

Adding to default.services.yml is a good idea, moving back to needs work for that.

Added the param to default.services.yml

LGTM.

Maybe wait until @alexpott confirms a services.yml file in sites/default is enough to take away his concerns in #27?

We can start here - if it proves a problem then we can visit. Ideally if you're on cheap limited hosting you don't have to edit files to do a migration. I recently set up D11 on some cheap hosting it has 2GB memory limit in PHP so maybe Pantheon will up their game... 128Mb seems tiny.

Yeah I think this is fine for now. For drush migrations it will start a new batch if it runs into trouble, the more common situation I've seen on Pantheon is regular hook_update_N() and config import running out of memory, which this may or may not help but we'll find out.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11383

Automatically closed - issue fixed for 2 weeks with no activity.