Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
routing system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Jul 2015 at 08:14 UTC
Updated:
11 Aug 2015 at 14:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
dawehnerDiff%
Diff
(microsec)
Diff%
MemUse
Diff
(bytes)
Diff%
PeakMemUse
Diff
(bytes)
Diff%
Comment #2
dawehnerGood catch fabianx!
Comment #4
fabianx commentedMicro-optimization, but it is well known that str_replace is faster than strtr, so lets do that.
Comment #5
wim leersShould we also file an upstream issue?
Comment #6
wim leersThis is difficult to parse.
Can we put these on separate lines, to remove the visual suggestion that this is a key-value map?
Comment #7
pwolanin commentedMaybe better to use a list() construct here? I think it's clearer to read.
Comment #8
dawehnerSure, why not.
Comment #9
wim leersRTBC+1
Comment #10
alexpottSo what's actually faster... if we're doing a micro-optimisation adding an extra operation seem interesting.
Comment #11
fabianx commented#10:
(http://php.net/manual/en/function.list.php)
So this should be identical to using:
However I benchmarked the cases and this is the result:
with the following scenarios (xhprof off):
So yes, list() is slightly slower than directly using decodedChars[0] and decodedChars[1], but is the same when using temporary variables.
Obviously the single case is fasted, but any optimization is likely not worth the additional complexity of the isset(), etc. and part of that is also saving one array traversal.
So it is probably just the 5x improvement over str_tr that really matters here - unless we never have a use-case to support more than one encoded character.
Comment #12
dawehnerThank you fabian! Good to learn that.
Given with that we support both cases I would suggest to go with that, just to add it to the documentation that you can also use arrays. It is quite an edge case to override it though.
Comment #13
fabianx commentedOh, that is a very sweet solution :).
Comment #14
alexpottThanks everyone! Committed 3555e2b and pushed to 8.0.x. Thanks!
Thank you for adding the beta evaluation to the issue summary.
Comment #16
fabianx commented