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?

CommentFileSizeAuthor
#6 3497954-6-2-x.patch1.16 KBcatch

Issue fork webform-3497954

Command icon Show commands

Start 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:

About issue forks

Comments

weseze created an issue. See original summary.

weseze’s picture

weseze commented

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...

catch made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

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

catch opened merge request !656

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3497954-6-2-x.patch1.16 KB

Adding a patch for a 6.2.x backport.

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3278736: Make loading of per webform additional js/css optional
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Posted some feedback.

catch’s picture

catch
he/him
Primary language English
commented

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’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

I'm good with this. LGTM

cilefen’s picture

cilefen commented
Title: Out of memory because of too large library definitions » Excessive memory use when building dynamic library definitions
jrockowitz’s picture

jrockowitz commented

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

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

There is an unresolved thread; @catch, @heddn

catch’s picture

catch
he/him
Primary language English
commented

Went ahead and resolved the thread per #9.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Tests are not passing.

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3542470: The config key_store (entity queries) can't handle long key values

Opened #3542470: The config key_store (entity queries) can't handle long key values.

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.

jrockowitz’s picture

jrockowitz commented

@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.

catch’s picture

catch
he/him
Primary language English
commented

@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.

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review

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.

catch’s picture

catch
he/him
Primary language English
commented

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.

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

jrockowitz’s picture

jrockowitz commented

Please review the MR.

Below are the changes and improvements.

  • On webform save and delete, track which webforms have CSS/JS libraries defined.
  • Use the same logic for clearing library definitions via \Drupal\webform\EntitySettings\WebformEntitySettingsAssetsForm::save and \Drupal\webform\Form\AdminConfig\WebformAdminConfigLibrariesForm::submitForm.
  • Add global CSS/JS routes (/webform/assets/global.css and /webform/assets/global.js).
  • Add \Drupal\webform\Controller\WebformAssetsController with fast_404 support.
  • Update \Drupal\webform\Hook\WebformLibrariesHooks::libraryInfoBuild to create global and webform specific CSS/JS libraries.
  • Remove duplicate code for attached webform assets \Drupal\webform\WebformSubmissionForm::attachLibraries.

Questions

  • Are there any major concerns with this improvement?
  • Are we okay with /webform/assets/global.css and /webform/jassetst/global.js paths?
catch’s picture

catch
he/him
Primary language English
commented

One option I can think of other than the global assets path routes would be to write the contents to a file in the public files system and then include this file in the library definition - this would allow for better browser caching and also aggregation. Whether it would be simpler or not I don't know.

Didn't profile/test this but the rest looks good to me apart from one comment on the upgrade path.

jrockowitz’s picture

jrockowitz commented

For now, I'd like to keep webform dynamic JS prefxed with /webform/*

  • /webform/assets/global.css
  • /webform/assets/global.js paths?
  • /webform/javascript/{webform_id}/custom.js
  • /webform/css/{webform_id}/custom.css
catch’s picture

catch
he/him
Primary language English
commented

Yeah I think that's fine and could always be changed later. The MR as it stands should significantly improve PHP memory usage when there's a lot of webforms, including when none of the CSS or JS features are used (whether per-webform or global)

jrockowitz’s picture

jrockowitz commented
Status: Needs review » Reviewed & tested by the community

Moving to RTBC. We will need to address the merge conflicts

  • jrockowitz committed 3b9c7f7c on 6.3.x authored by catch 
    [#3497954] feat: Excessive memory use when building dynamic library...
jrockowitz’s picture

jrockowitz commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • 74870f98 committed on 6.3.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...
jrockowitz’s picture

jrockowitz commented

I am not sure how I missed it but adding css/js lookup keys caused the below error. for any webform with css and js.

SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'name' at row 1: INSERT INTO "key_value"
("name", "collection", "value") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2);

Since the fix is two line change, I decided to fail foward and updated 6.3.x to get tests passing.

I also committed this tweak to '3497954-out-of-memory'.

  • 74870f98 committed on 6.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...

  • jrockowitz committed 3b9c7f7c on 6.x authored by catch 
    [#3497954] feat: Excessive memory use when building dynamic library...
catch’s picture

catch
he/him
Primary language English
commented

The lookup keys were due to #3497954-16: Excessive memory use when building dynamic library definitions and #17, I didn't realise they were still in the newer versions of the MR so was temporarily confused by #32. Those can indeed just be removed as lookup keys now that it's using state.

  • a77006e3 committed on 6.3.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...

  • ed3d87ba committed on 6.3.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...

  • ed3d87ba committed on 6.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...

  • a77006e3 committed on 6.x 
    Issue #3497954 by weseze, catch, heddn, cilefen, jrockowitz, liam...

Status: Fixed » Closed (fixed)

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

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

This has lead to #3571688: Custom CSS/JS is not attached when webform is rendered using block plugin. A partial revert is proposed in that issue. Please review.