Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem
- Yaml\Yaml instantiates a new Parser instance for every call to Yaml::parse() and a new Dumper instance for every call to Yaml::dump().
- It also performs a filestat for every call to Yaml::parse(), in order to check whether $input refers to a file.
Goal
- Improve performance of FileStorage.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 1713564-10.drupal8.config.perf-filestorage.patch | 4.67 KB | alexpott |
| #4 | 1713564-4.drupal8.config.perf-filestorage.patch | 22.97 KB | alexpott |
| #4 | 0-4-interdiff.txt | 18.24 KB | alexpott |
| config.perf-filestorage.0.patch | 2.24 KB | sun | |
Comments
Comment #2
alexpottconfig.perf-filestorage.0.patch queued for re-testing.
Comment #3
sunThis patch is the necessary prerequisite for using a custom indentation level of 2 spaces in our YAML files:
https://github.com/symfony/symfony/pull/5157
Comment #4
alexpottThe attached patch bumps Yaml Symfony component to master which includes @sun's https://github.com/symfony/symfony/pull/5157... so this patch is not commit-able until that pull request is tagged. And we can change /core/composer.json and composer.lock accordingly.
It also converts all existing Yaml file to have two spaces of indentation instead of the default 4.
Comment #5
sunWe already discussed in IRC... #4 is a little out of scope for this issue. ;)
Any chance to move forward with #0 ?
Comment #6
alexpottTentatively making patch in #0 rtbc as this works as expected.
The other thing I might do here is remove the static functions decode, encode etc... from the StorageInterface. These are no longer necessary.
Comment #7
catchSorry I don't quite get this.
Why are we instantiating the FileStorage class itself more than once? If we weren't, then instead of a static property for the dumper, it could just be a class property.
Comment #8
sunUm, I suspect some confusion here.
The FileStorage class is only instantiated once.
This issue/patch is about the Yaml Dumper+Parser. The FileStorage calls into the static Yaml class for every file it operates on.
And it is the static Yaml class that instantiates a new Dumper and new Parser for every call to Yaml::Dump() and Yaml::Parse().
The patch in #0 eliminates the repetitive instantiation of Dumper and Parser objects by making FileStorage instantiate a single instance only once and retaining that as a FileStorage class property.
As @alexpott pointed out, we will have to do that anyway at a later point, in order to leverage my PR to Symfony that allows us to define/control the indentation level of spaces in YAML files being dumped/written.
Does that clarify it?
Comment #9
catchIf FileStorage is only instantiated once, then those class properties don't need to be static?
Comment #10
sunoh. That's because the ::encode() and ::decode() methods are static, having the purpose of allowing other code to call into the bare encoding/decoding methods of a particular storage controller.
We might change those methods to be no longer static at some point, and if we do, then we can also un-static the getParser() and getDumper() methods. But that needs its own discussion. Until then, PHP would bail out if you try to access getParser()/getDumper() from the static encode()/decode() methods.
Comment #11
alexpottI think I ran a test removing the statics in the config StorageInterface and it works fine... so maybe this issue can just do this?
Comment #13
alexpott#11: 1713564-10.drupal8.config.perf-filestorage.patch queued for re-testing.
Comment #14
sun#11: 1713564-10.drupal8.config.perf-filestorage.patch queued for re-testing.
Comment #15
sunSlightly surprised that this still passes, but yay, back to RTBC :)
@catch's concerns about the static methods have been addressed in it.
Comment #16
sunCreated follow-up issue that has been mentioned earlier in here: #1768484: Indentation in YAML files violates Drupal coding standards
Comment #17
catchThanks. Committed/pushed to 8.x.