8000 [Routing] Don't needlessly execute strtr's as they are fairly expensive by arjenm · Pull Request #18257 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Don't needlessly execute strtr's as they are fairly expensive #18257

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 31, 2016
Merged

[Routing] Don't needlessly execute strtr's as they are fairly expensive #18257

merged 1 commit into from
Mar 31, 2016

Conversation

arjenm
Copy link
Contributor
@arjenm arjenm commented Mar 21, 2016
Q A
Branch? 2.7
Bug fix? refactor
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets see discussion in #18230
License MIT
Doc PR see #18230

As requested in #18230 this is a new version of the prevention of using strtr's. I've posted some performance-numbers in that PR as well.

fabpot
fabpot reviewed Mar 22, 2016
View reviewed changes
src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -76,6 +76,11 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
);

/**
* @var string This regexp matches all characters that are not or should not be encoded by rawurlencode (see list in array above).
*/
protected $urlEncodingSkipRegexp = '#[^-.~a-zA-Z0-9_/@:;,=+!*|]#';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @stof commented in the other PR, this property must be private even if the other ones are protected (the rules for private vs protected changed since then).

@arjenm
Copy link
Contributor Author
arjenm commented Mar 22, 2016

Ok, I changed the visibility of that property :)

@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

Thank you @arjenm.

@fabpot fabpot merged commit b3da6a1 into symfony:2.7 Mar 31, 2016
fabpot added a commit that referenced this pull request Mar 31, 2016
@fabpot
minor #18257 [Routing] Don't needlessly execute strtr's as they are f…
f03dc6e 
…airly expensive (arjenm)

This PR was merged into the 2.7 branch.

Discussion
----------

[Routing] Don't needlessly execute strtr's as they are fairly expensive

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | refactor
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | see discussion in #18230
| License       | MIT
| Doc PR        | see #18230

As requested in #18230 this is a new version of the prevention of using strtr's. I've posted some performance-numbers in that PR as well.

Commits
-------

b3da6a1 [Routing] Don't needlessly execute strtr's as they are fairly expensive
@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

FYI, I've fixed CS in ba1cd26

@Tobion
Copy link
Contributor
Tobion commented Mar 31, 2016

I'm definitely 👎 on this. The gain is negligible (0.005 ms for 1000 URLs in best case scenario as you can see in the test metrics). These changes make the code worse and is not future proof as it easily changes with PHP versions.

@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

Are you sure about the gains. I remembered a much better improvement from the other PR. Going to double-check, but if the gain is indeed small like you say, I will revert this PR.

@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

Actually, I can't find the test metrics you are referring to.

@Tobion
Copy link
Contributor
Tobion commented Mar 31, 2016

#18230 (comment)

@arjenm
Copy link
Contributor Author
arjenm commented Mar 31, 2016

I've looked at the code again. It sais ms, but I actually don't multiply by 1000... so its actually seconds and therefore a 1000 times more gain (i.e. 5ms rather than 0.005ms) than you quoted here. I'll adjust the test scripts to actually show ms.

@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

This talks about gains of more than 80/90% or am I reading this the wrong way?

@arjenm
Copy link
Contributor Author
arjenm commented Mar 31, 2016

@fabpot I suppose @Tobion was referring to the numbers shown on the test output's, I just summarized to percentages. Here are the scripts (now with actual ms, rather than seconds shown with a ms-postfix).
http://achelois.tweakers.net/~acm/urlrawencode_or_test_first.php
http://achelois.tweakers.net/~acm/dotsreplace_or_test_first.php
http://achelois.tweakers.net/~acm/queryreplace_or_test_first.php

@fabpot
Copy link
Member
fabpot commented Mar 31, 2016

I think I get it now. It takes 7ms to generate 1000 URLs, so, even if the gain in percentage is huge, on a typical webpage that has 100 URLs, that's still negligible.

@arjenm
Copy link
Contributor Author
arjenm commented Mar 31, 2016

There are three changes, of which two will be affecting every url generated and the third only if there is a query-parameter.
So that would be about 0.7ms+0.7ms for a 'good case' with that typical 100 url page.

We render most of our pages in about 30-100ms. So that would result in the 1-5% range of the average total response time.

I agree its not a huge gain.

@Tobion
Copy link
Contributor
Tobion commented Mar 31, 2016

It just feels odd to me too add a preg_match to skip a rawurlencode/strtr. What could be tested as well is if it is faster to replace the ``rawurlencode/strtrwith apreg_replace` and a callback (manual percent encoding of only the chars we want).

Tobion
Tobion reviewed Mar 31, 2016
View reviewed changes
src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -76,6 +76,11 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
);

/**
* @var string This regexp matches all characters that are not or should not be encoded by rawurlencode (see list in array above).
*/
private $urlEncodingSkipRegexp = '#[^-.~a-zA-Z0-9_/@:;,=+!*|]#';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be have been an @internal constant. it is not changing, so no point in making it a property

@arjenm
Copy link
Contributor Author
arjenm commented Mar 31, 2016

Well, the rawurlencode is actually quite fast, its mostly the strtr I wanted to prevent. But I put your idea to the test as well. And it does indeed work well, the overhead of the callback is fairly insignificant for my easy url's.

http://achelois.tweakers.net/~acm/urlrawencode_or_preg_replace.php

preg_replace_callback('#[^-a-zA-Z0-9._~/@:;,=+!*|]+#', function($matches){return rawurlencode($matches[0]);}, $url);

All examples are faster now, but with a slighlty smaller maximum gain. It does seem to be a (much) safer variant in terms of performance improvement. In terms of code complexity, it seems roughly similar but more logical.

@arjenm
Copy link
Contributor Author
arjenm commented Mar 31, 2016

I'll create a new PR for that variant, its similar in best-case performance, has likely better worst-case performance and is a bit cleaner.

@arjenm arjenm deleted the faster_url_generator_part1 branch March 31, 2016 11:41
This was referenced Mar 31, 2016
Closed
Closed
@fabpot
Copy link
Member
fabpot commented Apr 28, 2016

reverted

fabpot added a commit that referenced this pull request Apr 28, 2016
@fabpot
Revert "minor #18257 [Routing] Don't needlessly execute strtr's as th…
616ebb7 
…ey are fairly expensive (arjenm)"

This reverts commit f03dc6e, reversing
changes made to 2f2ce3e.
fabpot added a commit that referenced this pull request May 3, 2016
@fabpot
Merge branch '2.7' into 2.8
a5e6172 
* 2.7:
  add @event annotation for AuthenticationEvents
  add @event annotation for KernelEvents
  bumped Symfony version to 2.7.13
  updated VERSION for 2.7.12
  update CONTRIBUTORS for 2.7.12
  updated CHANGELOG for 2.7.12
  bumped Symfony version to 2.3.41
  updated VERSION for 2.3.40
  update CONTRIBUTORS for 2.3.40
  updated CHANGELOG for 2.3.40
  Revert "minor #18257 [Routing] Don't needlessly execute strtr's as they are fairly expensive (arjenm)"
  Revert "fixed CS"
  [FrameworkBundle] Remove misleading comment
  bug #17460 [DI] fix ambiguous services schema
fabpot added a commit that referenced this pull request May 3, 2016
@fabpot
Merge branch '2.8' into 3.0
2877f4b 
* 2.8:
  add @event annotation for AuthenticationEvents
  bumped Symfony version to 2.8.6
  [PropertyInfo] PHPDoc correction
  add @event annotation for KernelEvents
  updated VERSION for 2.8.5
  updated CHANGELOG for 2.8.5
  bumped Symfony version to 2.7.13
  updated VERSION for 2.7.12
  update CONTRIBUTORS for 2.7.12
  updated CHANGELOG for 2.7.12
  bumped Symfony version to 2.3.41
  updated VERSION for 2.3.40
  update CONTRIBUTORS for 2.3.40
  updated CHANGELOG for 2.3.40
  Revert "minor #18257 [Routing] Don't needlessly execute strtr's as they are fairly expensive (arjenm)"
  Revert "fixed CS"
  fixed deprecation notices in tests
  [Security] Normalize "symfony/security-acl" dependency versions across all composer.json files
  [FrameworkBundle] Remove misleading comment
  bug #17460 [DI] fix ambiguous services schema
fabpot added a commit that referenced this pull request May 3, 2016
@fabpot
Merge branch '3.0'
cab3f45 
* 3.0: (24 commits)
  add @event annotation for AuthenticationEvents
  bumped Symfony version to 3.0.6
  updated VERSION for 3.0.5
  updated CHANGELOG for 3.0.5
  bumped Symfony version to 2.8.6
  [PropertyInfo] PHPDoc correction
  add @event annotation for KernelEvents
  Fixed typo
  updated VERSION for 2.8.5
  updated CHANGELOG for 2.8.5
  bumped Symfony version to 2.7.13
  updated VERSION for 2.7.12
  update CONTRIBUTORS for 2.7.12
  updated CHANGELOG for 2.7.12
  bumped Symfony version to 2.3.41
  updated VERSION for 2.3.40
  update CONTRIBUTORS for 2.3.40
  updated CHANGELOG for 2.3.40
  Revert "minor #18257 [Routing] Don't needlessly execute strtr's as they are fairly expensive (arjenm)"
  Revert "fixed CS"
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Routing Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arjenm @fabpot @Tobion @javiereguiluz @carsonbot
0