Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Yaml] save preg_match() calls when possible#28106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedAug 1, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | possibly#27960 |
| License | MIT |
| Doc PR |
xabbuh commentedAug 1, 2018
This is how it looks like: Parsing the
(see Executing the Yaml component test suite:
(seehttps://blackfire.io/profiles/compare/b2f2e6a6-87d3-40ad-95dd-52a2ba6bdda7/graph) |
fabpot commentedAug 2, 2018
Thank you@xabbuh. |
This PR was merged into the 4.2-dev branch.Discussion----------[Yaml] save preg_match() calls when possible| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | possibly#27960| License | MIT| Doc PR |Commits-------e6bea97 save preg_match() calls when possible
staabm commentedAug 2, 2018
@xabbuh why does it save so much time in IOWait? The changes are not IO related.. |
| $context ='sequence'; | ||
| if (isset($values['value']) &&self::preg_match('#^&(?P<ref>[^ ]+) *(?P<value>.*)#u',$values['value'],$matches)) { | ||
| if (isset($values['value']) &&'&' ===$values['value'][0] &&self::preg_match('#^&(?P<ref>[^ ]+) *(?P<value>.*)#u',$values['value'],$matches)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it possible to have$values['value'] being the empty string here or no ? If yes, we have a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, the regular expression that has to match to reach this part of the code is#^\-((?P<leadspaces>\s+)(?P<value>.+))?$#u. So if thevalue element is set, it must have at least on character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah, thevalue group can indeed not be empty when it is matched, so things are fine.
| privatefunctionisBlockScalarHeader():bool | ||
| { | ||
| return (bool)self::preg_match('~'.self::BLOCK_SCALAR_HEADER_PATTERN.'$~',$this->currentLine); | ||
| return'' !==$this->currentLine &&(bool)self::preg_match('~'.self::BLOCK_SCALAR_HEADER_PATTERN.'$~',$this->currentLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what about adding&& \in_array($this->currentLine[0], array('|', '>'), true) && here too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought about that too, but the regular expression should also match lines likefoo: | orfoo: >2, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmm indeed. This regex is not anchored at the start. I missed that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just remembered that I indeed tried to add that check and was wondering now why tests didn't fail then. Turns out this method is totally useless with the current code (see#28187).
stof commentedAug 2, 2018
@staabm probably cold vs warm OPCache cache (and so bypassing the IO for many files) |
xabbuh commentedAug 2, 2018
@staabm I think what@stof said. You can see updated profile comparisons athttps://blackfire.io/profiles/compare/9de46b73-d563-4e0e-a81a-553cc3d5f290/graph (parsing the |