[Yaml] Base the parser on a StringReader #19750

GuilhemN · 2016-08-26T16:13:31Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes kind of
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

This PR proposes the introduction of a StringReader in the Yaml component (could be in its own component later if we are satisfied).
It's a huge change (no more regex and offset management in the parser) so I only updated some simple part of the parser for now to show how it could be used.

This would simplify the Yaml parser maintenance by removing regexs but also by using a clearer syntax (no more obvious function calls), the first feature I imagine is allowing custom tags (what about !service foo instead of @foo for dependency injection ?).

WDYT?

GuilhemN · 2016-08-26T21:12:20Z

I think I won't adapt more things in this PR as it become very quickly hard to review and I would like to do that step by step in case it is accepted :)

staabm · 2016-08-27T08:50:18Z

A perf comparison would be great.

GuilhemN · 2016-08-27T09:50:43Z

@staabm the tests are ~50ms slower on my computer (they took 250ms) but i think they should be faster or the same as currently once the parser will be entirely converted (the StringReader is instantiated several times for now and some variables are instantiated only because other parts aren't converted yet).
The most important performance gain should be on the biggest regex of the Parser (not converted yet).

BTW I think flexibility is the most important part for this component as its result is cached most of the time.

Edit: I think we could improve performances with a StringReaderGenerator that would replace things like eat('foo') by isset($data[$offset+2]) && 'f' === $data[$offset] && 'o' === $data[$offset+1] && 'o' === $data[$offset+2] and eatAny(array('foo', 'fah', 'bar')) by return isset($data[$offset+2]) ? ('f' === $data[$offset] ? ('o' === $data[$offset+1] && 'o' === $data[$offset+2] ? 'foo' : ($data[$offset+1] === 'a' && $data[$offset+2] === 'h' ? 'fah')) : ('b' === $data[$offset] && 'a' === $data[$offset+1] && 'r' === $data[$offset+2] ? 'bar' : false))) : false;.
That's a bit hard to read but if it is automatically generated it doesn't matter much.

We could as well generate ranges and hardcode them [a-zA-Z] can easily be hardcoded to abc...zABC...Z. But we would be forced to use commands to generate codes in the core. WDYT @fabpot ?

GuilhemN · 2016-08-28T10:46:52Z

Maybe you would prefer a real feature directly ? (such as custom tags)

javiereguiluz · 2016-08-29T07:39:41Z

@Ener-Getick after a quick review of your proposal there is one (minor) thing I don't like: the name of the eat*() methods. Could we replace them by read*()? e.g. eatWhiteSpace() -> readWhiteSpace()

GuilhemN · 2016-08-29T09:30:36Z

@javiereguiluz how would you call eat, eatAny, eatSpan and eatCSpan? readString, readAny, readSpan and readCSpan ?

javiereguiluz · 2016-08-29T09:38:16Z

@Ener-Getick yes, exactly like that :)

sstok · 2016-08-29T09:40:34Z

src/Symfony/Component/Yaml/Inline.php

     *
     * @return string A YAML string
     *
     * @throws ParseException When malformed inline YAML string is parsed
-     *
-     * @internal


If this was marked as internal in 2.x then you can even make this method private, no?
If it was marked as internal in 3.x it may not be possible to change it (just yet).

the entire class is @internal so I removed this tag ;)

This class is only internal since Symfony 3.1.

@xabbuh if someone looks at the 3.1 code he will see that this method is @internal because its class is. What's the problem?

But you're right, if it should be removed, it should be done in 3.1.

sstok · 2016-08-29T09:42:06Z

read(Any, Span, CSpan, String) and consumeWhiteSpace (that name is used in PHP-cs-fixer, could even leave out the White part).

sstok · 2016-08-29T09:44:04Z

src/Symfony/Component/Yaml/Inline.php

-            // quoted scalar
-            $output = self::parseQuotedScalar($scalar, $i);
+        if ($quote = $reader->eatSpan('"\'', 1)) {
+            $unescaper = new Unescaper();


Can this be injected / defined as a property?

Its methods are static, it could even not be instantiated.

javiereguiluz · 2016-08-30T06:37:45Z

I was looking at the StringReader implementations in .NET and in Java. Some comments in case they're useful:

Our readToFullConsumption() method is called readToEnd() in .NET. I like that.
Both APIs are based on two versions of read() method, one to read chars and other strings. It's a bit strange to use in our code readString('{') when you are reading just a single char.
.NET defines the peek() method, which works as read() but doesn't consume the char. I don't know if we need that.
Their constructors is simply StringReader(String) I don't know if we really need the start and end arguments.

GuilhemN · 2016-08-30T10:17:16Z

readToEnd looks good to me :)
I introduced readChar and readAnyChar, they work as readString and readAny but are a bit faster
I also introduced peek to remove a call to read + State::restore()
For your last point, we need these arguments during the transition phase as we use several instance of StringReader (see their usage here) edit: after all i used substr instead to optimize readSpan.

GuilhemN · 2016-09-06T17:42:22Z

Do you think it will be possible to have this in 3.2 or is it too late ?
I know it contains a lot of modifications so don't hesitate to ask me if you don't understand why I changed something.

GuilhemN · 2016-09-09T19:10:45Z

BTW this PR fixes several limitations:

tags are allowed before any values (eg !foo {my: map}), that's only useful for non-specific tags for now but it will be useful for custom tags if we allow them later
tags are allowed for keys ({ !str false: bar })
mark to be deprecated several things (it is not deprecated yet):
- unsupported tags
- every reserved indicators
- using consecutive commas in mappings (eg {foo: bar,,})
- resolvable keys (eg false: should be converted to ! false: as false is not a supported key in php)
fix support of spaces in keys ({foo bar: quz} wasn't possible)

xabbuh · 2016-09-27T18:18:54Z

I am not against this change per se especially if it greatly increases readability. However, we should be sure not to decrease performance as this component is used by many packages (of which many probably do not use a caching system like Symfony is using it when building the DI container).

GuilhemN · 2016-09-27T19:22:44Z

@xabbuh i don't think people should use this component for its performance (there is the yaml extension for that) but instead for its extensibility and its integration with symfony.

Tests are ~10% slower on my computer. Maybe we can improve this more (blackfire attributes a big cost to functions not doing anything themselves, such as readSpan, maybe caused by blackfire itself or because of something else?).
To chose whether to merge this, I think we also have to take into account the ability to fix bugs and add new features more easily as well as having a more spec compliant parser (for example, maps and sequences could be parsed the same way according to the parser, only the output changes).

GuilhemN · 2016-10-30T09:12:55Z

I guess we now have to wait for 3.3?

I don't know if i should continue to add changes here as it would only make the diff bigger but i still think we can really improve the yaml component by changing its internals.
Do you want me to change something else or is this gonna be rejected because it is slower than currently?

xabbuh · 2016-12-04T19:07:21Z

I don't have a really strong opinion either way. Though it would be good to get some more opinion of the other @symfony/deciders before investing more time here IMO.

dunglas · 2016-12-05T07:22:52Z

Having a true parser instead of relying on regex is a good idea.

However @xabbuh is right, I saw a lot of PHP applications using this component to parse YAML files at runtime and without cache. Using the C extension is often not an option because it is not broadly available and requires to be able to install packages on the server.
It's one of the most popular Symfony component, performance must not decrease here.

stof · 2016-12-05T14:19:12Z

@GuilhemN can you share a blackfire comparison of this PR vs master (and maybe the profiles themselves too, at least for the PR) ?

Btw, most usage of readSpan seems to come from consuming whitespaces. It might be worth looking at optimizing them

stof · 2016-12-05T14:21:58Z

Thus, most places consuming whitespaces don't care about them, so you could have a skipWhitespaces instead of consumeWhitespaces for them, which would move the cursor without actually reading the content of these bytes (so doing the strspn call but not calling internalRead with the result)

GuilhemN · 2016-12-05T17:43:04Z

I used this benchmark based on the config.yml file of the Symfony standard edition.

The first try was 34% slower, then with the consumeWhitespace optimization, it was 18% slower than currently (you can find the last profile here).

theofidry · 2016-12-05T18:17:27Z

@GuilhemN I'm getting a 404 for your link (latest profile) :/

GuilhemN · 2016-12-05T18:22:21Z

@theofidry sorry, I forgot to make them public... it should work now.

neclimdul · 2016-12-06T22:03:43Z

src/Symfony/Component/Yaml/Inline.php

+
+            // bc support of [ foo, , ]
+            while ($reader->readChar(',')) {
+                $reader->consumeWhiteSpace();


This section looks like one of those rare cases where a do {} while() makes sense.

stof · 2016-12-07T16:02:34Z

@GuilhemN please use public links for the comparisons too

GuilhemN · 2016-12-07T16:15:04Z

It looks like the links were broken this time...

javiereguiluz · 2016-12-07T16:15:30Z

@GuilhemN are you still using the benchmark that you mentioned in this comment?

for ($i = 0; $i < 10000; ++$i) {
    $parser->parse('[foo, bar, quz: foo, {bar: foo, quz: mou,}]');
}

I don't like that benchmark because it parses a small YAML file thousands of times. I prefer something more realistic, like parsing a medium-sized YAML file a few times. For example, you could use the Symfony Demo app/config/config.yml file and parse 5 times or so.

GuilhemN · 2016-12-07T16:18:04Z

@javiereguiluz i did not like it too, i changed it to this one, based on the standard edition.
It is still executed 1000 times, should I change it?

javiereguiluz · 2016-12-07T16:19:19Z

@GuilhemN OK thanks! I still think that parsing the YAML file 1,000 times is very unrealistic.

GuilhemN · 2016-12-07T16:29:39Z

@javiereguiluz the results when executed 5 times: before / after / comparison.

theofidry · 2016-12-07T16:54:50Z

@GuilhemN to be more fair in your benchmark you could profile:

parse a small file
parse a medium size file
parse a big file

You could do so separately or within the same process. You could also use Blackfire prob to exclude the autoloading from the profile which can always be kinda falsy cf. Blackfire doc.

GuilhemN · 2016-12-07T17:22:16Z

@theofidry I don't think that's worth it, a benchmark will never be exact anyway. The goal is to see the slowest parts of the code and see the global trend when changing a big part of the code; it's good enough as is imo :)

For the autoloader, I'll run more samples at the same time, the results should be more accurate.

GuilhemN · 2017-05-18T16:56:19Z

I'm closing this as it needs a lot of work to be rebased and most of the things it fixed we're fixed otherwise.
The win in readability would be great but smaller changes must be easier to review/accept, eventually I'll open smaller PRs.

carsonbot added Status: Needs Review Yaml Feature labels Aug 26, 2016

GuilhemN force-pushed the STRINGREADER branch 8 times, most recently from 6c187fc to 8947a4e Compare August 26, 2016 19:19

GuilhemN force-pushed the STRINGREADER branch from 038a94e to ab88a58 Compare August 26, 2016 21:18

GuilhemN changed the title ~~[Yaml] Add a StringReader~~ [Yaml] Base the parser on a StringReader Aug 26, 2016

GuilhemN force-pushed the STRINGREADER branch 2 times, most recently from fc7dd89 to fe6901b Compare August 28, 2016 13:50

sstok reviewed Aug 29, 2016
View reviewed changes

GuilhemN force-pushed the STRINGREADER branch from 3e3cbb7 to 51b87f7 Compare September 1, 2016 12:25

GuilhemN mentioned this pull request Sep 6, 2016

[RFC][Yaml] Consider colon not followed by spaces as a value #19874

Closed

javiereguiluz added this to the 3.3 milestone Nov 6, 2016

GuilhemN force-pushed the STRINGREADER branch from e1a948d to e607582 Compare December 3, 2016 21:09

neclimdul reviewed Dec 6, 2016

View reviewed changes

GuilhemN force-pushed the STRINGREADER branch from 06d0a98 to c00d8e2 Compare December 21, 2016 21:37

[Yaml] Add a StringReader

f25f7f2

GuilhemN force-pushed the STRINGREADER branch from c00d8e2 to f25f7f2 Compare December 26, 2016 12:41

GuilhemN closed this May 18, 2017

Uh oh!

[Yaml] Base the parser on a StringReader #19750

[Yaml] Base the parser on a StringReader #19750

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!