Out of memory because of too large library definitions

Created on 7 January 2025, 8 months ago

We have a large drupal setup, containing quite a few modules (contrib and custom), running 100+ sites.
PHP is set up with a memory limit of 256Mb.
A few weeks ago some sites started running out of memory on a cold cache.

After some investigation, the culprit was the "cache_discovery" of the "library_info:our_custom_theme".
It seems the library definition calculation has become so large for some of our sites that it needs 400Mb to calculate.
We have temporarily set our PHP memory_limit to 512Mb to mitigate the issue.

We did more investigation by trying to figure out which module was causing the most memory build-up and it turned out webform was responsible for half of the peak memory usage.

In Drupal\Core\Cache\CacheCollectorInterface\LibraryDiscovery::getLibrariesByExtension() we added a piece of code to test this:

  public function getLibrariesByExtension($extension) {
    if ($extension === 'webform') {
      return [];
    }

    ... original code ...

  }

Before this test we had a memory peak usage (cold cache) of 395Mb.
After the test only 195Mb.

I understand that 150Mb probably comes from our setup and we could look into that. But most likely we will end up concluding that there are no easy wins here.

Is this 200Mb of memory usage something that is expected to build the library cache for webform?
If so, how can we "fix" this so peak memory usage is way less?

πŸ› Bug report
Status

Needs work

Version

6.3

Component

Code

Created by

πŸ‡§πŸ‡ͺBelgium weseze

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @weseze
  • πŸ‡§πŸ‡ͺBelgium weseze

    I did some more digging and found the issue is the "webform_library_info_build()".
    This function loads all webform, checks them for custom CSS/JS and adds those as a library if needed.

    The website where we noticed the issue has 1000+ webforms. So trying to load them all leads to 200Mb of memory usage.

    I tried changing the code to load them 1-by-1 and clear the static entity caching between each, but this didn't help. The memory usage stayed the same. I thought this would have been a quick and easy "fix"... :(

    The better solution would be to split the code in 2 pieces:
    1/ general CSS/JSS assets need to be added once in a "shared" library for all webforms
    2/ only webforms that have specific CSS/JSS assets need to be loaded: this could still be an issue if there are many: not sure how to address that...

  • First commit to issue fork.
  • πŸ‡¬πŸ‡§United Kingdom catch

    The issue here is loading every webform and then looping over them to check if they have css or javascript.
    Config entities support entity queries, so it's possible to pre-filter with an entity query. For me this reduced the time taken for this hook from five seconds to about 230ms

  • πŸ‡¬πŸ‡§United Kingdom catch

    Adding a patch for a 6.2.x backport.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • Pipeline finished with Failed
    about 1 month ago
    Total: 632s
    #551477
  • heddn Nicaragua

    Posted some feedback.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Double checked and ::exists() definitely isn't an option here - it results in loading all the webforms in, so I think the <> '' logic is correct here.

  • heddn Nicaragua

    I'm good with this. LGTM

  • πŸ‡ΊπŸ‡ΈUnited States jrockowitz Brooklyn, NY

    The problem and solution makes a lot of sense to me. I am good with it

  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦

    There is an unresolved thread; @catch, @heddn

  • πŸ‡¬πŸ‡§United Kingdom catch

    Went ahead and resolved the thread per #9.

  • Pipeline finished with Failed
    26 days ago
    Total: 650s
    #558835
  • Pipeline finished with Failed
    4 days ago
    Total: 954s
    #576879
  • πŸ‡¨πŸ‡¦Canada Liam Morland Ontario, CA πŸ‡¨πŸ‡¦

    Tests are not passing.

  • Pipeline finished with Failed
    2 days ago
    Total: 687s
    #578460
  • πŸ‡¬πŸ‡§United Kingdom catch

    I had a look into the test failures.

    The problem is in the way that config entity queries work.

    The key value entries look like this:

    | config.entity.key_store.webform_options           | uuid:eba38bf8-26e1-49ef-87f8-5093b78cf9f8 | a:1:{i:0;s:33:"webform.webform_options.education";} 
    

    webform.webform.test_cards_javascript.yml has a very long string of JavaScript in it.

    The key_store tries to put the full value of the javascript string into 'name' which is not possible given it's varchar 128.

    If we truncated this to varchar 128 in core it should work, so I will open an issue to discuss that. This might need to be postponed but would prefer to see feedback for the core issue is like first, there might be other ways to handle this if that specific change to core isn't doable.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Opened πŸ› The config key_store (entity queries) can't handle long key values Active .

    If that issue isn't fixable in a way that helps here, another possible approach would be to add 'has_js' and 'has_css' keys to webform config, obviously without a UI, then populate them based on the js/css keys - these would only need boolean values so would then fit in the config entity key_store and be queryable, and we could simplify the queries here.

    I think this issue is severe enough that would be worth doing, but it would be a lot more work - would need an upgrade path etc.

  • πŸ‡ΊπŸ‡ΈUnited States jrockowitz Brooklyn, NY

    @catch Could we store which webforms has js/css libraries via state?

    On webform create, update and delete, we can update the 'webform_libraries' state value.

    In 'webform_libraries' state value we could store the webform ids or the entire library definition.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @jrockowitz yeah that could be a good option!

    Storing the entire library definition in state could be heavy now because state is cached with a cache collector, but could use key/value directly instead of state - this is only used when building library definitions so doesn't really need to be chained fast/state as such. But also if it's only a list of which webforms have libraries that would definitely be small enough for state.

    Either way would be simpler than adding the new keys to the config entities.

  • πŸ‡ΊπŸ‡ΈUnited States jrockowitz Brooklyn, NY

    Please review the MR for storing webform libraries via state. The MR accounts for sites that have custom CSS/JS for all webforms.

    We could move the global CSS/JS into a dedicated library attached seperately.

  • Pipeline finished with Failed
    2 days ago
    Total: 468s
    #578619
  • πŸ‡¬πŸ‡§United Kingdom catch

    We could move the global CSS/JS into a dedicated library attached seperately.

    That sounds like a good idea to avoid OOM for sites that have configured global CSS/JS. I thought I had a way around this when I commented on the MR but if that global css/js currently has to be added to the individual webform library definitions then I don't think there's an alternative to splitting it into its own thing.

    A separate library for that would also help with asset aggregate hit rates (e.g. one library for all webforms, instead of unique one for each webform but with the same actual CSS/JS in it) - at least if I'm understanding what the differences are - not actually using the feature anywhere that I know of.

  • πŸ‡ΊπŸ‡ΈUnited States jrockowitz Brooklyn, NY

    jrockowitz β†’ changed the visibility of the branch 3497954-store-webform-with-css-js-in-state to hidden.

Production build 0.71.5 2024