Bug #53862

Epic #55070: Workpackages

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

Bug #52949: Speed decrease since 4.5

isValidUrl idna converts whole URI instead of domain only

Added by Michiel Roos almost 6 years ago. Updated about 1 year ago.

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

100%

TYPO3 Version:
6.2
PHP Version:
5.3
Tags:
Complexity:
easy
Is Regression:
No
Sprint Focus:

Description

The expensive idna_convert is called from isValidUrl. Instead of feeding it just the domain part, it converts the whole URI.

When feeding it just the domain part, a great speed gain can be had.

This patch breaks apart the URI using parse_url(), converts the domain part, then re-assembles the uri using HttpUtility::buildUrl().

Please find profiling runs attached for:
  • /typo3/ (login screen)
  • /typo3/backend.php
  • /typo3/mod.php?M=web_list

backend_php.png View (34.2 KB) Michiel Roos, 2013-11-22 12:05

login_screen.png View (34.4 KB) Michiel Roos, 2013-11-22 12:05

web_list.png View (34.7 KB) Michiel Roos, 2013-11-22 12:05

fu_idna.v3.diff View (1.01 KB) Michiel Roos, 2013-11-22 12:05

fu_idna_4.7.patch View - patch for 4.7 branch (1.07 KB) Michiel Roos, 2013-11-22 13:01

patch_set_6_backend_php.png View (36.8 KB) Michiel Roos, 2013-11-22 15:52

inValidFiles_login_page.txt View - Non valid urls tested again from the login page (6.48 KB) Michiel Roos, 2013-11-23 01:06

Screen_Shot_2013-11-23_at_12.08.54.png View - Patch set 14 performance measurement (38.9 KB) Michiel Roos, 2013-11-23 12:09

TYPO3_CMS_Login__6.2.master.workbench.local___Workbench-7.jpg View - http://例え.テスト/typo3/ (139 KB) Michiel Roos, 2013-11-26 19:51

Screen_Shot_2013-11-26_at_20.03.58.png View (102 KB) Michiel Roos, 2013-11-26 20:05

Patch_25_backendPhp_unpopulated_Cache_dir.png View (35.4 KB) Michiel Roos, 2013-12-20 11:26

Patch_25_dbList_siteRoot_populated_Cache_dir.jpg View (45.4 KB) Michiel Roos, 2013-12-20 11:26

Patch_25_login_unpopulated_Cache_dir.jpg View (43.6 KB) Michiel Roos, 2013-12-20 11:26


Related issues

Related to TYPO3 Core - Bug #55475: Error when saving Domain system record Closed 2014-01-30
Related to TYPO3 Core - Bug #55713: GeneralUtility' not found Closed 2014-02-06

Associated revisions

Revision f8fdcea7 (diff)
Added by Michiel Roos almost 6 years ago

[BUGFIX] isValidUrl() idna converts whole URI

GeneralUtility::isValidUrl() idna converts whole URI instead of
domain only.

The expensive idna_convert() is called from isValidUrl(). Instead of
feeding it just the domain part, the whole URI is converted.

When supplying just the domain part, a great speed gain can be seen.

On seriously malformed URLs, parse_url() may return FALSE and emits an
E_WARNING. So we check for that first.

PHP no longer supports the flags FILTER_FLAG_HOST_REQUIRED and
FILTER_FLAG_SCHEME_REQUIRED. A scheme is now required by default. [1]
Return FALSE if the URL does not start with a scheme.

A public GeneralUtility::idnaEncode() method uses a static idna_convert
instance and fetches converted strings from a string cache array
to avoid multiple checks on the same domain.

All manual idna_convert instances are replaced with
GeneralUtility::idnaEncode() calls.

Special characters are not allowed in the URL except in the domain
part [2]. So the test with special characters in the path was removed
from the GeneralUtilityTest class.

[1] http://www.php.net/manual/en/filter.filters.flags.php#107382
[2] http://tools.ietf.org/html/rfc3986#appendix-A

Change-Id: I7a0ab0a399d9d6cf68c824f413be6b6d621947c1
Resolves: #53862
Releases: 6.2, 6.1, 6.0
Reviewed-on: https://review.typo3.org/25636
Reviewed-by: Markus Klein
Reviewed-by: Wouter Wolters
Tested-by: Wouter Wolters
Reviewed-by: Michiel Roos
Tested-by: Michiel Roos
Tested-by: Markus Klein
Reviewed-by: Andreas Wolf
Reviewed-by: Jo Hasenau
Reviewed-by: Xavier Perseguers
Tested-by: Xavier Perseguers

Revision c99a07a9 (diff)
Added by Michiel Roos almost 6 years ago

[BUGFIX] isValidUrl() idna converts whole URI

GeneralUtility::isValidUrl() idna converts whole URI instead of
domain only.

The expensive idna_convert() is called from isValidUrl(). Instead of
feeding it just the domain part, the whole URI is converted.

When supplying just the domain part, a great speed gain can be seen.

On seriously malformed URLs, parse_url() may return FALSE and emits an
E_WARNING. So we check for that first.

PHP no longer supports the flags FILTER_FLAG_HOST_REQUIRED and
FILTER_FLAG_SCHEME_REQUIRED. A scheme is now required by default. [1]
Return FALSE if the URL does not start with a scheme.

A public GeneralUtility::idnaEncode() method uses a static idna_convert
instance and fetches converted strings from a string cache array
to avoid multiple checks on the same domain.

All manual idna_convert instances are replaced with
GeneralUtility::idnaEncode() calls.

Special characters are not allowed in the URL except in the domain
part [2]. So the test with special characters in the path was removed
from the GeneralUtilityTest class.

[1] http://www.php.net/manual/en/filter.filters.flags.php#107382
[2] http://tools.ietf.org/html/rfc3986#appendix-A

Change-Id: I7a0ab0a399d9d6cf68c824f413be6b6d621947c1
Resolves: #53862
Releases: 6.2, 6.1, 6.0
Reviewed-on: https://review.typo3.org/26501
Reviewed-by: Xavier Perseguers
Tested-by: Xavier Perseguers
Reviewed-by: Markus Klein
Tested-by: Markus Klein

Revision d0c42764 (diff)
Added by Michiel Roos almost 6 years ago

[BUGFIX] isValidUrl() idna converts whole URI

GeneralUtility::isValidUrl() idna converts whole URI instead of
domain only.

The expensive idna_convert() is called from isValidUrl(). Instead of
feeding it just the domain part, the whole URI is converted.

When supplying just the domain part, a great speed gain can be seen.

On seriously malformed URLs, parse_url() may return FALSE and emits an
E_WARNING. So we check for that first.

PHP no longer supports the flags FILTER_FLAG_HOST_REQUIRED and
FILTER_FLAG_SCHEME_REQUIRED. A scheme is now required by default. [1]
Return FALSE if the URL does not start with a scheme.

A public GeneralUtility::idnaEncode() method uses a static idna_convert
instance and fetches converted strings from a string cache array
to avoid multiple checks on the same domain.

All manual idna_convert instances are replaced with
GeneralUtility::idnaEncode() calls.

Special characters are not allowed in the URL except in the domain
part [2]. So the test with special characters in the path was removed
from the GeneralUtilityTest class.

[1] http://www.php.net/manual/en/filter.filters.flags.php#107382
[2] http://tools.ietf.org/html/rfc3986#appendix-A

Change-Id: I7a0ab0a399d9d6cf68c824f413be6b6d621947c1
Resolves: #53862
Releases: 6.2, 6.1, 6.0
Reviewed-on: https://review.typo3.org/26531
Reviewed-by: Xavier Perseguers
Tested-by: Xavier Perseguers
Reviewed-by: Markus Klein
Tested-by: Markus Klein

Revision 3dcc61df (diff)
Added by Wouter Wolters over 5 years ago

[BUGFIX] Regression in DataHandler

The fix for issue #53862 calls GeneralUtility::idnaEncode
without fully qualified class namespace.

Follow-up to: c99a07a9

Resolves: #55475
Releases: 6.1, 6.0
Change-Id: I8ba161ee73e7456da53d2182b4a22d87dad9d53c
Reviewed-on: https://review.typo3.org/27173
Reviewed-by: Steffen Müller
Tested-by: Steffen Müller
Reviewed-by: Christian Kuhn
Tested-by: Christian Kuhn

Revision dc271e4f (diff)
Added by Wouter Wolters over 5 years ago

[BUGFIX] Regression in DataHandler

The fix for issue #53862 calls GeneralUtility::idnaEncode
without fully qualified class namespace.

Follow-up to: c99a07a9

Resolves: #55475
Releases: 6.1, 6.0
Change-Id: I8ba161ee73e7456da53d2182b4a22d87dad9d53c
Reviewed-on: https://review.typo3.org/27174
Reviewed-by: Christian Kuhn
Tested-by: Christian Kuhn
Reviewed-by: Wouter Wolters
Tested-by: Wouter Wolters

History

#1 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/25636

#2 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/25636

#4 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/25636

#5 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/25636

#6 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/25636

#7 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/25636

#9 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/25636

#10 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/25636

#11 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/25636

#12 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/25636

#13 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/25636

#14 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/25636

#15 Updated by Michiel Roos almost 6 years ago

These files are tested but come back invalid.

We may gain some more speed by finding out who is testing them and why.

#16 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/25636

#18 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/25636

#19 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/25636

#20 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/25636

#21 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/25636

#23 Updated by Michiel Roos almost 6 years ago

Basic test against:

/typo3/backend.php from a 'just cleared the cache' state.

#24 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/25636

#25 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/25636

#26 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/25636

#27 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/25636

#28 Updated by Gerrit Code Review almost 6 years ago

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

#29 Updated by Gerrit Code Review almost 6 years ago

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

#30 Updated by Gerrit Code Review almost 6 years ago

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

#31 Updated by Gerrit Code Review almost 6 years ago

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

#33 Updated by Gerrit Code Review almost 6 years ago

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

#34 Updated by Michiel Roos almost 6 years ago

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

#35 Updated by Markus Klein almost 6 years ago

  • Parent task set to #52949

#36 Updated by Benni Mack about 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF