Performance issue with Folder->updateChildFilesystem

[sb-relaxt-at]

I guess I stumbled on some performance issue that became a real issue due to silverstripe/silverstripe-framework#9162.

A Folder does call updateChildFilesystem even after a skipped write. Given that there are lots of nested Folders and Files this can be a very expensive call. I am wondering if this is really necessary if there is no change in the Folder itself?

We investigated this issue after updating to CMS 4.5.0 with the mentioned change. Suddenly writing a page took 10-100 times longer. Each Page has a has_one relation to a Folder. Before writing a Page each Page's parent Page is accessed as well as the associated Folder. Therefore both are loaded components and are written. This results in all parent Pages and their Folders being written (recursively up the site tree). Although these Folders are unchanged, they do trigger a updateChildFilesystem resulting in even more reads and writes.

[emteknetnz]

Hi, thanks for reporting this

Would I be able to get you to clarify something?
Each Page has a has_one relation to a Folder
Is this relation defined in your custom codebase? Or is there a Silverstripe module that defines this relationship?

[sb-relaxt-at]

It's a custom module. One that ensures that there is an associated folder for each page so that assets are properly stored in a structure reassembling the site tree.

I assume the issue itself is independent from this structure, but our code is triggering it.

[blueo]

Yeah we're running into problems with the change too. We have a many many through object that has an Image attached, the Image also has a Taxonomy Term. Writing the through object will cause the file, its parent folder, any assets in that folder (355 in this case) and the associated taxonomy term and all child terms of that term (again in the 100s) to write.

The asset write seems to be the biggest performance issue. Some of our asset folders have many hundreds of images which will effectively break editing for any asset contained in that folder.

[blueo]

@sminnee just wondering why the writeComponent = true was set for all calls - it seems to be the trigger for these problems

[sminnee]

Ooh. This was to support relationship editing within grid field detail forms. Either has-one relation or other records on the join-record if you're editing a many-many relationship. It was part of making more sophisticated ModelAdmin apps. But it sounds like it's got some unintended consequences.

Perhaps this setting needs to be amended so that it's a config property on the GridFieldDetailForm for the given grid-field. If we did that I'd probably revert the default to false, the behaviour in 4.4.

Arguably, we could patch this in 4.5 because this is essential a bugfix. We could also revert #9164 in 4.5 as the bugfix and introduce the new config option in 4.6.

[maxime-rainville]

@blueo Can you check what happens if you comment out this line?

silverstripe-assets/src/Folder.php

Line 268 in 7bba9d2

$this->updateChildFilesystem();

It's not 100% clear to me why we need this call if the write is skipped.

[sminnee]

I'd also like to note that this is the 2nd bug my change introduced, so even if we fix this where Max has identified, downgrading component-writing to an optional feature of the GridFieldDetailForm might still be a good idea.

[sminnee]

PS: @blueo you might want to use https://github.com/cweagans/composer-patches to revert my change on your project while we're still getting this addressed in a stable release. Hit me up on Slack if you have questions.

[maxime-rainville]

I commented out the line and ran the assets unit test locally. It didn't break anything, so at first glance, if this line is needed, it's not covered by any test.

[blueo]

@maxime-rainville commenting out $this->updateChildFilesystem(); improves the time to complete the save (presumably not waiting for filesystem operations is a good thing) but there is another problem - the recursive write seems to cascade to everything attached to the item.

In our current bug we're writing a through object which has an asset attached. The asset has a taxonomy term. What's happening is the asset writes, it then causes a write on its parent folder, which causes a write on all items in that folder (many hundreds in this case). It also writes the taxonomy term which in turn writes all connected terms (parent or child). All of this generates a bunch of queries (some of which will be from application logic but not all).

If I run a profile after commenting out updateChildFilesystem and some application onAfterWrite calls it still calls SilverStripe\ORM\Queries\SQLExpression::execute ~41,000 times. If I revert the change in GridFieldDetailForm_ItemRequest to $this->record->write(false, false, false, false), then it only generates 136 calls.

[sminnee]

As the creator of the original change I’d like to go on record as saying changing the default was a mistake and it should be a config flag on the gridfield component.

If people agree, should we fix in 4.5 or 4.6?

[blueo]

The effect is probably too far reaching for us to safely use - there are many gridfield interactions we'd need to check off, a config would be a good way to do it I think.

[chrispenny]

If we are considering this a "bug" (which, I feel it is), then could the fix please go into 4.5.x?

[sminnee]

Yeah, to have one behaviour in 4.4 and 4.6 and a different behaviour in 4.5 would be helpful to no one.

[HJGreen]

Thanks for the work on this issue - any idea when there might be a new 4.5.x release with this fix included?
silverstripe/silverstripe-framework#9528

It's blocking an upgrade to 4.5, and we're getting a little concerned with 4.4 becoming EOL next week.

@sminnee Thanks for the suggestion to patch the framework, however we're unable to get the patch to apply as part of the CWP build process - we have a support ticket open with them regarding this.

[maxime-rainville]

@HJGreen We'll be releasing 4.6.0 in the next 2-3 weeks. We'll be tagging patch releases for 4.4 and 4.5 at the same time.

[emteknetnz]

Closing as this was fixed in silverstripe/silverstripe-framework#9528