Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Yaml] Optimise Inline::evaluateScalar() for parsing strings. #10312

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

The Drupal 8 installer does a lot of YAML parsing. With the patch attached it is significantly quicker.
image

https://drupal.org/node/1851234

Found another optimisation. Happy to split this into a separate PR if you think it is worth it.

Again this has quite a big impact on the Drupal installer.
image

@dawehner dawehner commented on the diff Feb 23, 2014

src/Symfony/Component/Yaml/Inline.php
return false;
- case is_numeric($scalar):
- return '0x' == $scalar[0].$scalar[1] ? hexdec($scalar) : floatval($scalar);
- case 0 == strcasecmp($scalar, '.inf'):
- case 0 == strcasecmp($scalar, '.NaN'):
- return -log(0);
- case 0 == strcasecmp($scalar, '-.inf'):
- return log(0);
- case preg_match('/^(-|\+)?[0-9,]+(\.[0-9]+)?$/', $scalar):
- return floatval(str_replace(',', '', $scalar));
- case preg_match(self::getTimestampRegex(), $scalar):
- return strtotime($scalar);
+ // Optimise for returning strings.
+ case $scalar[0] === '+' || $scalar[0] === '-' || $scalar[0] === '.' || $scalar[0] === '!' || is_numeric($scalar[0]):
@dawehner

dawehner Feb 23, 2014

Contributor

Is there a reason to not just use in_array(..., TRUE)?

@alexpott

alexpott Feb 23, 2014

It is slower. Not hugely - but in_array is more expensive
image

@wouterj

wouterj Feb 23, 2014

Member

it's better to split the case statements:

case $scalar[0] === '+' :
case $scalar[0] === '-':
case $scalar[0] === '.':
case $scalar[0] === '!':
case is_numeric($scalar[0]):
Contributor

dawehner commented Feb 23, 2014

Given that the Inline class has quite some test coverage I think we don't need additional coverage.

Owner

fabpot commented Feb 23, 2014

Thanks @alexpott.

@fabpot fabpot added a commit that referenced this pull request Feb 23, 2014

@fabpot fabpot minor #10312 [Yaml] Optimise Inline::evaluateScalar() for parsing str…
…ings. (alexpott)

This PR was submitted for the 2.3-dev branch but it was merged into the 2.3 branch instead (closes #10312).

Discussion
----------

[Yaml] Optimise Inline::evaluateScalar() for parsing strings.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The Drupal 8 installer does a lot of YAML parsing. With the patch attached it is significantly quicker.
![image](https://f.cloud.github.com/assets/769634/2237906/630c3308-9bcf-11e3-8038-35cbbeedd7e6.png)

https://drupal.org/node/1851234

Commits
-------

10c898d Avoid unnecessary line indentation calculation.
c65a647 Optimise Inline::evaluateScalar() for parsing strings.
6b0b504

@fabpot fabpot closed this Feb 23, 2014

@Tobion Tobion commented on the diff Feb 24, 2014

src/Symfony/Component/Yaml/Inline.php
- return '0x' == $scalar[0].$scalar[1] ? hexdec($scalar) : floatval($scalar);
- case 0 == strcasecmp($scalar, '.inf'):
- case 0 == strcasecmp($scalar, '.NaN'):
- return -log(0);
- case 0 == strcasecmp($scalar, '-.inf'):
- return log(0);
- case preg_match('/^(-|\+)?[0-9,]+(\.[0-9]+)?$/', $scalar):
- return floatval(str_replace(',', '', $scalar));
- case preg_match(self::getTimestampRegex(), $scalar):
- return strtotime($scalar);
+ // Optimise for returning strings.
+ case $scalar[0] === '+' || $scalar[0] === '-' || $scalar[0] === '.' || $scalar[0] === '!' || is_numeric($scalar[0]):
+ switch (true) {
+ case 0 === strpos($scalar, '!str'):
+ return (string) substr($scalar, 5);
+ case 0 === strpos($scalar, '! '):
@Tobion

Tobion Feb 24, 2014

Member

Why not use $scalar[0] === '!' here as well?

@sun

sun Feb 24, 2014

Contributor

The strpos() check would still be required for differentiating between !str and !, and all other cases, so that change would not present a perf improvement.

@stof

stof Feb 24, 2014

Member

@Tobion have you missed the space after ! ?

@Tobion

Tobion Feb 24, 2014

Member

Indeed I have.

@Tobion Tobion commented on the diff Feb 24, 2014

src/Symfony/Component/Yaml/Inline.php
+ }
+
+ return null;
+ case ctype_digit($scalar):
+ $raw = $scalar;
+ $cast = intval($scalar);
+
+ return '0' == $scalar[0] ? octdec($scalar) : (((string) $raw == (string) $cast) ? $cast : $raw);
+ case '-' === $scalar[0] && ctype_digit(substr($scalar, 1)):
+ $raw = $scalar;
+ $cast = intval($scalar);
+
+ return '0' == $scalar[1] ? octdec($scalar) : (((string) $raw == (string) $cast) ? $cast : $raw);
+ case is_numeric($scalar):
+ return '0x' == $scalar[0].$scalar[1] ? hexdec($scalar) : floatval($scalar);
+ case 0 == strcasecmp($scalar, '.inf'):
@Tobion

Tobion Feb 24, 2014

Member

Why not '.inf' === $scalarLower?
Also it should be 0 === strcasecmp when using this camparison.

@sun

sun Feb 24, 2014

Contributor

'.inf' === $scalarLower sounds like a very good idea → let's create a follow-up? :)

@sun

sun Feb 25, 2014

Contributor

See #10323

@sun sun added a commit to sun/symfony that referenced this pull request Feb 25, 2014

@sun sun Follow-up to #10312: Fixed minor performance related issues in Yaml\I…
…nline.
a36fef5

@fabpot fabpot added a commit that referenced this pull request Feb 25, 2014

@fabpot fabpot minor #10323 [Yaml] Fixed minor performance related issues in Yaml\In…
…line. (sun)

This PR was submitted for the 2.3-dev branch but it was merged into the 2.3 branch instead (closes #10323).

Discussion
----------

[Yaml] Fixed minor performance related issues in Yaml\Inline.

Follow-up to #10312

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

a36fef5 Follow-up to #10312: Fixed minor performance related issues in Yaml\Inline.
ca60916

@fabpot fabpot added a commit that referenced this pull request Feb 27, 2014

@fabpot fabpot Merge branch '2.3' into 2.4
* 2.3:
  bumped Symfony version to 2.3.12
  updated VERSION for 2.3.11
  update CONTRIBUTORS for 2.3.11
  updated CHANGELOG for 2.3.11
  Follow-up to #10312: Fixed minor performance related issues in Yaml\Inline.

Conflicts:
	src/Symfony/Component/HttpKernel/Kernel.php
cb4a9cc

@fabpot fabpot added a commit that referenced this pull request Mar 3, 2014

@fabpot fabpot Merge branch '2.4'
* 2.4:
  [Form][2.3] Fixes empty file-inputs getting treated as extra field.
  changed some PHPUnit assertions to more specific ones
  fixed Kernel::stripComments() normalizing new-lines
  added a BC comment
  Update FileLoader to fix issue #10339
  bumped Symfony version to 2.3.12
  updated VERSION for 2.3.11
  update CONTRIBUTORS for 2.3.11
  updated CHANGELOG for 2.3.11
  Throw exception when unable to normalize embedded object
  Fixed evaluation of short circuit operators
  Follow-up to #10312: Fixed minor performance related issues in Yaml\Inline.
  [2.4][HttpKernel] Fix issue #10209 When the profiler has `only_exception` option activated and a subrequest throw an exception, the parent profile cannot be found.
77bfac7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment