Skip to content
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.

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

Performance optimization by replacing is_null with === null #201

Closed
wants to merge 3 commits into from
Closed

Performance optimization by replacing is_null with === null #201

wants to merge 3 commits into from

Conversation

SchumacherFM
Copy link
Member

According to http://framework.zend.com/issues/browse/ZF-5413 the usage of is_null is slow.
So I replaced every occurrences of is_null or !is_null with === null or !== null

Unit test are running "normal".

First time for me, so I do accept your Contributor License Agreement.

PS: This pull request contains all commits. Sorry.

@magento-team
Copy link
Contributor

Have you tried benchmarking this change? we have previously tried to change even more significant parts of the code but in the whole rendering time that speed difference was negligible (even though if the code itself repeated many times was much faster)

@mtdowling
Copy link

A quick google search shows that the overhead of the is_null function call vs using === null does make a small difference. Magento is such a large framework that you shouldn't turn down these really easy micro-optimizations.

<?php

$i = $argv[1];
$a = null;

$s = microtime(true);
for ($j = 0; $j < $i; $j++) {
    is_null($a);
}
echo (microtime(true) - $s) . "\n";

$s = microtime(true);
for ($j = 0; $j < $i; $j++) {
    $a === null;
}
echo (microtime(true) - $s) . "\n";

Benchmark:

$ php ~/projects/perf/is_null_vs_null.php 100000
0.15766286849976
0.034035205841064

@magento-team
Copy link
Contributor

Hello. We did a small test and it showed that there is no noticeable improvement for real page rendering and is_null is more readable. We appreciate your work on this contribution but we will not accept it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants