Task #54265

Epic #55070: Workpackages

Epic #55065: WP: Overall System Performance (Backend and Frontend)

Story #55078: Optimize PHP code performance in TYPO3 methods

Use (int) instead of intval() or (integer)

Added by Jo Hasenau almost 6 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
Category:
Performance
Target version:
Start date:
2013-12-07
Due date:
% Done:

100%

TYPO3 Version:
6.2
PHP Version:
5.4
Tags:
Complexity:
Sprint Focus:

Description

Description

When testing for the identity of integers using the intval() function has worse performance than a type cast with (int). (3-5x)

Replace all ~1600 calls to intval() with a type cast.


Related issues

Related to TYPO3 Core - Task #54085: Replace all strcmp() calls with === Closed 2013-11-29
Related to TYPO3 Core - Bug #55753: pagetype is not respected in pagecache Closed 2014-02-07
Related to TYPO3 Core - Bug #61699: Workspace Preview not working with lockSSL=3 Closed 2014-09-18
Precedes TYPO3 Core - Bug #55706: Different behavior of levelmedia in TYPO3 6.2 Closed 2014-02-05

Associated revisions

Revision 9646d26b (diff)
Added by Jo Hasenau almost 6 years ago

[TASK] Use (int) instead of intval() or (integer)

This patch replaces most of around 1600 occurrences of
intval() and every (integer) in the whole core.
Additionally it changes GeneralUtility::intExplode to use references
and typecasting as well.
Some occurrences of strstr() together with intval() have been replaced
with strpos() as well.
And some superfluous intval calls have been removed or reduced
to a single one i.e. for protected variables or before loops.
Patch updated after Core CGL decision from 30.01.2014

Resolves: #54265
Releases: 6.2
Change-Id: Iba57ffad1f4233ffa1a9f7d3ca5bfe9d2b53f1e8
Reviewed-on: https://review.typo3.org/26740
Reviewed-by: Stefan Neufeind
Tested-by: Stefan Neufeind

Revision ede69ff6 (diff)
Added by Oliver Hader about 5 years ago

[BUGFIX] Workspace Preview not working with lockSSL=3

The workspace preview does not work with lockSSL=3 being defined
in the Install Tool. First it looks like a CSS issue, in the end
it boils down that the preview IFRAMEs are not correctly
initialized in Workspaces\Hook\TypoScriptFrontendControllerHook.

The mentioned hook is only called once for the request to the
first IFRAME showing the website frontend preview and won't be
called for further.

The reason is, that cache expire headers are sent that prevent
the client to update the preview on each preview request.
That's why the mentioned hook is not called and the IFRAMEs
are not initialized properly. No-Cache headers are sent if a
backend user object is initialized correctly - and that's the
actual bug, a wrong but strict PHP condition, comparing a string
(lockSSL) with an integer value.

The comparison flaw was integrated with issue #54265

Change-Id: I13c5c2f22f5b43b49f8eead88f1bc82daf415cbb
Resolves: #61699
Releases: master, 6.2
Reviewed-on: http://review.typo3.org/32851
Reviewed-by: Wouter Wolters <>
Reviewed-by: Markus Klein <>
Tested-by: Markus Klein <>
Reviewed-by: Oliver Hader <>
Tested-by: Oliver Hader <>

Revision 9c0ba3a9 (diff)
Added by Oliver Hader about 5 years ago

[BUGFIX] Workspace Preview not working with lockSSL=3

The workspace preview does not work with lockSSL=3 being defined
in the Install Tool. First it looks like a CSS issue, in the end
it boils down that the preview IFRAMEs are not correctly
initialized in Workspaces\Hook\TypoScriptFrontendControllerHook.

The mentioned hook is only called once for the request to the
first IFRAME showing the website frontend preview and won't be
called for further.

The reason is, that cache expire headers are sent that prevent
the client to update the preview on each preview request.
That's why the mentioned hook is not called and the IFRAMEs
are not initialized properly. No-Cache headers are sent if a
backend user object is initialized correctly - and that's the
actual bug, a wrong but strict PHP condition, comparing a string
(lockSSL) with an integer value.

The comparison flaw was integrated with issue #54265

Change-Id: I13c5c2f22f5b43b49f8eead88f1bc82daf415cbb
Resolves: #61699
Releases: master, 6.2
Reviewed-on: http://review.typo3.org/32857
Reviewed-by: Oliver Hader <>
Tested-by: Oliver Hader <>

History

#1 Updated by Jo Hasenau almost 6 years ago

Actually the impact is around factor 2.5 not 3 to 5 but still it should be worth switching from intval() to (int).

There is one execpetion though, which is the "base" parameter of the intval() function.
Don't know yet, if this is actually used within the TYPO3 core, but if it is, then intval() should not be replaced.

#2 Updated by Markus Klein almost 6 years ago

Hi! Thanks for fiddling around with this.

Before we invest that much time here (the replacement with a regexp is fast, but the review is very cumbersome), we'd discuss this on the core list first.
I've a gut feeling that not too many devs will support this and therefor it might be hard to get it merged finally.

Cheers Markus

#3 Updated by Markus Klein almost 6 years ago

  • Parent task set to #52949

#4 Updated by Steffen Ritter almost 6 years ago

We discuessed this shortly on the release team meeting as we are concepting the performance improvements.

We are in - but need to make sure that it's really the same :)

#5 Updated by Mathias Schreiber almost 6 years ago

Hi joey,

in general: great idea.
I suggest changing the topic to "replace intval() with (int) where it makes sense" in order to make it sound less ranty

#6 Updated by Steffen Ritter almost 6 years ago

  • Subject changed from Replace all intval() with (int) to Replace all intval() with (int) wherever it makes sense

#7 Updated by Jo Hasenau almost 6 years ago

So I will go for it but do it the "semiautomatic" way.

It might take a bit more patchsets than #54085 but IMHO it was working quite well to have a limited number of changes per patchset. So reviewing will be easier and discussing can happen on the way through the patchsets. After all there might be less context related things to discuss than with strcmp().

BTW: According to some PHP benchmarking pages I have some more candidates like array_unique or array_key_exists on my list. So if you guys already found some others, feel free to assign the tasks to me.

#8 Updated by Stefan Froemken almost 6 years ago

Please correct me, but I may read that we want to use (integer) instead of (int). Same with (boolean) instead of (bool). I think we should use the same style everywhere.

#9 Updated by Jo Hasenau almost 6 years ago

Agreed to use the same style, but in that case it should be (int) and (bool) for everything, since these are less "bloaty" while still easily understandable.

#10 Updated by Stefan Neufeind almost 6 years ago

Please see current thread on the core-mailinglist about CGL-decisions like these ("integer or int"). There is a notes-document to sum up decisions, their pros/cons etc.

#11 Updated by Jo Hasenau almost 6 years ago

So could someone take care of that document and confirm one or the other way.
https://notes.typo3.org/p/CGL-decisions

It's not necessary to decide everything but at least the (bool) (int) vs. (boolean) (integer) thingie should be decided before I go on with this task.

#12 Updated by Gerrit Code Review almost 6 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#13 Updated by Gerrit Code Review almost 6 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#14 Updated by Gerrit Code Review almost 6 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#15 Updated by Gerrit Code Review almost 6 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#16 Updated by Gerrit Code Review almost 6 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#17 Updated by Chris topher almost 6 years ago

  • Subject changed from Replace all intval() with (int) wherever it makes sense to Use (integer) instead of intval() or (int)

#18 Updated by Gerrit Code Review almost 6 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#19 Updated by Gerrit Code Review almost 6 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#20 Updated by Gerrit Code Review almost 6 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#21 Updated by Gerrit Code Review almost 6 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#22 Updated by Gerrit Code Review almost 6 years ago

Patch set 10 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#23 Updated by Gerrit Code Review almost 6 years ago

Patch set 11 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#24 Updated by Gerrit Code Review almost 6 years ago

Patch set 12 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#25 Updated by Gerrit Code Review almost 6 years ago

Patch set 13 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#26 Updated by Gerrit Code Review almost 6 years ago

Patch set 14 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#27 Updated by Gerrit Code Review almost 6 years ago

Patch set 15 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#28 Updated by Ernesto Baschny almost 6 years ago

  • Parent task changed from #52949 to #55078

#29 Updated by Ingo Schmitt almost 6 years ago

Core decision as from today: it should be written (int)$variable

#30 Updated by Gerrit Code Review almost 6 years ago

Patch set 16 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#31 Updated by Gerrit Code Review almost 6 years ago

Patch set 17 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#32 Updated by Gerrit Code Review almost 6 years ago

Patch set 18 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#33 Updated by Bernhard Kraft almost 6 years ago

What are the (php internal?) differences between intval() and a (int) typecast?

Is it only that intval() takes a base argument (defaulting to 10) and the difference in speed or are there other specialities?

Could someone point me to a document on that case?

#34 Updated by Jo Hasenau almost 6 years ago

It's exactly that: A difference in speed and memory consumption.

The rule of thumb is:

  1. The right operator or typecast is better than
  2. the right native function is better than
  3. your own optimized method for the same purpose

So as long as there is no need for a different base, you should do a typecast to (int) instead of intval().

This might be irrelevant with future PHP versions, since the PHP guys might improve performance as well, but still we can be sure that the function call will never outperform the typecast.

http://www.php.net/manual/de/language.types.type-juggling.php
http://www.php.net/manual/de/language.types.integer.php#language.types.integer.casting

<quote>
To explicitly convert a value to integer, use either the (int) or (integer) casts. However, in most cases the cast is not needed, since a value will be automatically converted if an operator, function or control structure requires an integer argument. A value can also be converted to integer with the intval() function.
</quote>

#35 Updated by Gerrit Code Review almost 6 years ago

Patch set 19 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#36 Updated by Gerrit Code Review almost 6 years ago

Patch set 20 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#37 Updated by Gerrit Code Review almost 6 years ago

Patch set 21 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#38 Updated by Gerrit Code Review almost 6 years ago

Patch set 22 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/26740

#39 Updated by Jo Hasenau almost 6 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#40 Updated by Simon Schaufelberger almost 6 years ago

can somebody please share the script how to do that automatically or was everything done manually? I want to use that for my own extensions as well!

#41 Updated by Jo Hasenau almost 6 years ago

This was done manually, since I wanted to take care of surrounding issues as well, which would have been impossible with an automated search and replace.
The goal was to get rid of superfluous method calls and typecasts additionally. So the only helper was PHPstorm to find the occurrences and provide a local history of the changes.

#42 Updated by Chris topher over 5 years ago

  • Subject changed from Use (integer) instead of intval() or (int) to Use (int) instead of intval() or (integer)

#43 Updated by Riccardo De Contardi about 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF