Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We still have a few PHP 4-style type casting functions. Hinting the relevant type can be 300-600% times faster, and it's a straight forward fix. Please review if any of you have some time.
Steps to reproduce
Proposed resolution
Change the code and enable a phpcs rule to prevent the problem from reoccurring
Remaining tasks
This is postponed on the two child issues,
#3337254: Replace Updater.php intval() functions with their relevant type casting operators
#3337257: Replace the use of intval() and strval() as callbacks with a foreach and typecast or such
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|
Issue fork drupal-2931262
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Ayesh CreditAttribution: Ayesh commentedComment #7
jungleHi, Ayesh, very glad to meet an issue you created. I did do benchmarks by phpbench , with PHP 7.1, 7.2 and 7.3. The results show that 7.2 has improved performance. and no obvious diff between the two ways under the tested PHP versions.
So I doubt that casting is only faster under PHP 5. If so, I think it's optional to replace it nowadays. But it's good to unify the way in Core.
See the details below, and the script attached.
---
Benchmark under PHP 7.1
2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 1.587 [1.726 1.726] 1.604 (μs)
⅀T: 27.622μs μSD/r 0.053μs μRSD/r: 3.028%
suite: 1343abadc450cf699e4e3f4a86a6c3827d3bc82f, date: 2020-01-22, stime: 17:35:55
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 435,904b | 1.969μs | +1.00σ | +4.29% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 435,904b | 1.807μs | -1σ | -4.29% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 435,904b | 1.668μs | -1σ | -0.53% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 435,904b | 1.686μs | +1.00σ | +0.53% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 435,904b | 1.738μs | +1.00σ | +3.61% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 435,904b | 1.617μs | -1σ | -3.61% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 435,904b | 1.954μs | +1.00σ | +9.62% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 435,904b | 1.611μs | -1σ | -9.62% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 435,904b | 1.764μs | +1.00σ | +0.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 435,904b | 1.749μs | -1σ | -0.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 435,904b | 1.676μs | -1σ | -2.36% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 435,904b | 1.757μs | +1.00σ | +2.36% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 435,904b | 1.587μs | -1σ | -0.52% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 435,904b | 1.604μs | +1.00σ | +0.52% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 435,904b | 1.767μs | +1.00σ | +2.88% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 435,904b | 1.668μs | -1σ | -2.88% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
Benchmark result under PHP 7.2
2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.224 [0.250 0.250] 0.229 (μs)
⅀T: 4.006μs μSD/r 0.010μs μRSD/r: 3.633%
suite: 1343abaf04d23c21b9c9d7d5a0aa8e8d88101f71, date: 2020-01-22, stime: 17:37:04
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 458,512b | 0.278μs | -1σ | -2.06% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 458,512b | 0.290μs | +1.00σ | +2.06% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 458,512b | 0.224μs | -1σ | -3.49% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 458,512b | 0.240μs | +1.00σ | +3.49% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 458,512b | 0.241μs | -1σ | -1% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 458,512b | 0.246μs | +1.00σ | +1.00% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 458,512b | 0.230μs | -1σ | -3.42% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 458,512b | 0.246μs | +1.00σ | +3.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 458,512b | 0.249μs | -1σ | -15.14% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 458,512b | 0.338μs | +1.00σ | +15.14% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 458,512b | 0.225μs | -1σ | -3.29% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 458,512b | 0.241μs | +1.00σ | +3.29% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 458,512b | 0.251μs | +1.00σ | +0.59% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 458,512b | 0.248μs | -1σ | -0.59% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 458,512b | 0.229μs | +1.00σ | +0.06% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 458,512b | 0.228μs | -1σ | -0.06% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
Benchmark result under PHP 7.3
2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.228 [0.246 0.246] 0.233 (μs)
⅀T: 3.929μs μSD/r 0.004μs μRSD/r: 1.453%
suite: 1343aba58483a93833b8b6f36649ea59e46d26b2, date: 2020-01-22, stime: 17:37:42
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 462,432b | 0.247μs | -1σ | -0.78% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 462,432b | 0.251μs | +1.00σ | +0.78% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 462,432b | 0.228μs | -1σ | -5.99% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 462,432b | 0.257μs | +1.00σ | +5.99% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 462,432b | 0.250μs | +1.00σ | +0.39% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 462,432b | 0.248μs | -1σ | -0.39% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 462,432b | 0.233μs | +1.00σ | +0.19% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 462,432b | 0.232μs | -1σ | -0.19% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 462,432b | 0.246μs | -1σ | -0.79% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 462,432b | 0.250μs | +1.00σ | +0.79% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 462,432b | 0.245μs | -1σ | -1.8% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 462,432b | 0.254μs | +1.00σ | +1.80% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 462,432b | 0.248μs | +1.00σ | +1.20% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 462,432b | 0.242μs | -1σ | -1.2% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 462,432b | 0.250μs | +1.00σ | +0.49% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 462,432b | 0.248μs | -1σ | -0.49% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
Comment #8
jungleBTW, 3x9 what I meant is three bit of number 9. originally, I tested 999,999,999, in short 9x9
Comment #15
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
At this time we will need a D10 of this patch and also a recheck of the repo if there are other instances.
Please do not just reroll without checking.
Comment #17
Ayesh CreditAttribution: Ayesh commentedNew patch for Drupal 10.1.x branch, replacing about 50 intval calls.
Comment #18
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you. IF you open a MR you don't need to upload a patch fyi. Hiding the files to not confuse people
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedAfter applying patch #17 see 11 instances of intval still in the repo.
Comment #20
Ayesh CreditAttribution: Ayesh commentedThank you @smustgrave.
I had indeed missed a few
intval
calls, which I have now (force) pushed with fixes. There are twointval
calls remaining, but they use the second parameter (base) to convert them to octal numbers. We could convert them to a type cast followed by adecoct
call, but I think theintval
in those couple instances are fine.Comment #21
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed I'm only seeing the 2 intvals() in the updater.php file now.
Comment #22
xjmI think that if we're going to adopt this, we need a phpcs rule to keep these from creeping back in:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Pro...
Comment #23
Ayesh CreditAttribution: Ayesh commentedAdded a CS rule forbid said casting functions, but emit a warning and not an error. I don't know PHPCS enough to see if it can add a warning message instead of a function suggestion. The two instance of
intval
calls are explicitly allowed with a PHPCS toggle.The reason why I thought to use a warning instead of an error is because none of these functions are deprecated, and especially in
intval
case, there is a parameter for base conversion. If you think we are better off with an error, I'll happily change it too.Thank you.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks for adding the warning.
Confirmed it's working by editing a random test file and using the functions that I shouldn't and I did get the warning.
Comment #25
xjmBut will the warning cause commit checks to fail on DrupalCI, to prevent these from creeping back in?
Forking the MR to check.
Edit: BTW naming an MR branch 10.1.x is not a good idea; you should always prefix your feature branches with the issue ID. :)
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedIs it because they are warnings and not errors?
Comment #28
xjm@smustgrave Right, that's what I was trying to confirm. And it works:
https://www.drupal.org/pift-ci-job/2576189
Has:
I grepped for remaining usages:
For the usages in the updater, I think we should file a followup to consider converting those and remove the PHPCS ignore, because of this little bit of fun:
However, that still leaves these three places where
intval
is used as a callback, which presumably would still be faster if replaced with aforeach
and typecast or such:There are similarly any number of places where
strval()
is used as a callback:Since those callbacks are apparently not identified by the PHPCS rule, and since reviewing changes to rewrite an
array_mao()
is more complex than simply scanning the 1:1 replacements here, I think those usages should be scoped to a followup issue.So NW for two followup issues: One for the octal, and one for the use of
intval()
andstrval()
as callbacks.I also left one small point of feedback on the MR, but this mostly looks good.
Thanks!
Comment #29
lucasscComment #30
lucasscUnnecessary parentheses removed. Please, review.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedChanges look good
Moving to NW for the 2 followups requested by @xjm in #28
Thanks.
Comment #32
lucasscFollow-up issues created: #3337254: Replace Updater.php intval() functions with their relevant type casting operators and #3337257: Replace the use of intval() and strval() as callbacks with a foreach and typecast or such.
I guided myself with this doc, as it's the first time I've done it. I hope that's correct.
I believe the next step is to update the issue summary? Tagging for this.
Comment #33
longwaveAdded a number of code review comments.
Comment #34
lucasscHi, @longwave! Thanks for the code review.
I removed the unnecessary casts and followed your suggestions rewriting some lines in a cleaner way.
Just not sure about this change in core/lib/Drupal/Core/Updater/Updater.php:
Should we keep with '0755' as the 2nd argument?
$filetransfer->chmod($old_perms, 0755);
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedFollowing back up on this I believe the change should be fine? Will send it up to see if the committers have any thoughts too.
Comment #36
longwaveA couple of clarifications and nitpicks in the MR but otherwise this looks almost ready to go.
Comment #38
rpayanmApplied the @longwave's suggestions, please review.
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill seem some open threads
Comment #40
rpayanmWhich one? Sorry, I can't see anything :-(
Comment #41
smustgrave CreditAttribution: smustgrave at Mobomo commentedI think I was wrong here - previously this also protected against uid 0 being cancelled. We could still write something like
if ($uid == 1) {
$root = $account;
}
if ($uid <= 1) {
continue;
}
Comment #42
rpayanmThat code is already in the MR here.
Comment #43
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #44
quietone CreditAttribution: quietone at PreviousNext commentedComment #45
quietone CreditAttribution: quietone at PreviousNext commentedThere are two child issues which are fixing special cases of *val. They need to be committed before this one because this is enabling the rule.
I am postponing this.