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.
Files: 
CommentFileSizeAuthor
#11 1713564-10.drupal8.config.perf-filestorage.patch4.67 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 40,399 pass(es). View
#4 1713564-4.drupal8.config.perf-filestorage.patch22.97 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,832 pass(es). View
#4 0-4-interdiff.txt18.24 KBalexpott
config.perf-filestorage.0.patch2.24 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,807 pass(es). View

Comments

Status: Needs review » Needs work
Issue tags: -Performance, -Configuration system

The last submitted patch, config.perf-filestorage.0.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +Configuration system

config.perf-filestorage.0.patch queued for re-testing.

sun’s picture

Title: Instantiate Yaml\Dumper and Yaml\Parser only once » Make Config\FileStorage instantiate Yaml\Dumper and Yaml\Parser only once

This 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

alexpott’s picture

FileSize
18.24 KB
22.97 KB
PASSED: [[SimpleTest]]: [MySQL] 39,832 pass(es). View

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

sun’s picture

We already discussed in IRC... #4 is a little out of scope for this issue. ;)

Any chance to move forward with #0 ?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

sun’s picture

Um, 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?

catch’s picture

If FileStorage is only instantiated once, then those class properties don't need to be static?

sun’s picture

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

alexpott’s picture

FileSize
4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 40,399 pass(es). View

I think I ran a test removing the statics in the config StorageInterface and it works fine... so maybe this issue can just do this?

Status: Needs review » Needs work
Issue tags: -Performance, -Configuration system

The last submitted patch, 1713564-10.drupal8.config.perf-filestorage.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
sun’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Slightly surprised that this still passes, but yay, back to RTBC :)

@catch's concerns about the static methods have been addressed in it.

sun’s picture

Created follow-up issue that has been mentioned earlier in here: #1768484: Indentation in YAML files violates Drupal coding standards

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. Committed/pushed to 8.x.

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