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

Use isset() instead of strlen() #39

Merged
merged 3 commits into from Jan 18, 2015

Conversation

Projects
None yet
3 participants
@LukasReschke
Copy link
Contributor

LukasReschke commented Jan 18, 2015

isset() is way faster than counting the string length (due to being an op-code), furthermore counting a string's length is not really required unless you actually need the length somewhere.

Use isset() instead of strlen()
isset() is way faster than counting the string length, furthermore counting a string's length is not really required unless you actually need the length somewhere.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2015

Coverage Status

Coverage remained the same when pulling af84c0a on LukasReschke:use-isset-instead-of-strlen into 1daa878 on punic:master.

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 18, 2015

Do you have some benchmark of this? I always thought that strlen was as fast as reading a structure value...

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 18, 2015

isset() has it's own Opcode which at least in ZendVM makes a considerable speed difference:

<?php

session_write_close();
require_once('./3rdparty/punic/punic/punic.php');

$time1 = microtime(true);

$string = 'foo';
for ($i = 1; $i <= 10000000; $i++) {
    $bar = isset($string[0]);
}

$time2 = microtime(true);
echo 'isset time: ' . ($time2 - $time1);
echo '<br/>';

$time3 = microtime(true);

for ($i = 1; $i <= 10000000; $i++) {
    $bar = strlen($string);
}

$time4 = microtime(true);
echo 'strlen time: ' . ($time4 - $time3);
echo '<br/>';

isset time: 0.38738322257996
strlen time: 1.289724111557

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 18, 2015

Isset:

Finding entry points
Branch analysis from position: 0
Return found
filename:       /in/QJIOA
function name:  (null)
number of ops:  4
compiled vars:  !0 = $string, !1 = $bar
line     # *  op                           fetch          ext  return  operands
---------------------------------------------------------------------------------
   3     0  >   ASSIGN                                                   !0, 'foo'
   4     1      ISSET_ISEMPTY_DIM_OBJ                       2000000  ~1      !0, 0
         2      ASSIGN                                                   !1, ~1
         3    > RETURN                                                   1

Strlen:

Finding entry points
Branch analysis from position: 0
Return found
filename:       /in/GC2ZS
function name:  (null)
number of ops:  5
compiled vars:  !0 = $string, !1 = $bar
line     # *  op                           fetch          ext  return  operands
---------------------------------------------------------------------------------
   3     0  >   ASSIGN                                                   !0, 'foo'
   4     1      SEND_VAR                                                 !0
         2      DO_FCALL                                      1  $1      'strlen'
         3      ASSIGN                                                   !1, $1
         4    > RETURN                                                   1
@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 18, 2015

Really interesting. Thanks!

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 18, 2015

I just made a PR on your PR to fix some minor issues 😉

Merge pull request #1 from mlocati/use-isset-instead-of-strlen-fixes
Correct replacement of strlen with isset
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2015

Coverage Status

Coverage remained the same when pulling dca5f5b on LukasReschke:use-isset-instead-of-strlen into 1daa878 on punic:master.

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 18, 2015

I just made a PR on your PR to fix some minor issues 😉

Merged. Thanks!

mlocati added a commit that referenced this pull request Jan 18, 2015

@mlocati mlocati merged commit 7d2e5a9 into punic:master Jan 18, 2015

2 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 18, 2015

Great!

@LukasReschke LukasReschke deleted the LukasReschke:use-isset-instead-of-strlen branch Jan 18, 2015

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 21, 2015

I run this script to evaluate the checks to see if strings are empty:

@session_write_close();
foreach(
    array(
        'Test with empty string' => "",
        'Test with short string' => "short",
        'Test with long string' => str_repeat("This is a string that's quite long", 50),
    ) as $name => $string) {
    echo $name, "\n";

    $time1 = microtime(true);
    for ($i = 1; $i <= 10000000; $i++) {
        if(strcmp('', $string) !== 0) {
            ;
        }
    }
    $time2 = microtime(true);
    echo "\tstrcmp time: " . ($time2 - $time1), "\n";

    $time1 = microtime(true);
    for ($i = 1; $i <= 10000000; $i++) {
        if(strlen($string)) {
            ;
        }
    }
    $time2 = microtime(true);
    echo "\tstrlen time: " . ($time2 - $time1), "\n";


    $time1 = microtime(true);
    for ($i = 1; $i <= 10000000; $i++) {
        if($string != '') {
            ;
        }
    }
    $time2 = microtime(true);
    echo "\t!='' time  : " . ($time2 - $time1), "\n";

    $time1 = microtime(true);
    for ($i = 1; $i <= 10000000; $i++) {
        if($string !== '') {
            ;
        }
    }
    $time2 = microtime(true);
    echo "\t!=='' time : " . ($time2 - $time1), "\n";

    $time1 = microtime(true);
    for ($i = 1; $i <= 10000000; $i++) {
        if(isset($string[0])) {
            ;
        }
    }
    $time2 = microtime(true);
    echo "\tisset time : " . ($time2 - $time1), "\n";
}

And here's the result:

Test with empty string
        strcmp time: 19.674000024796
        strlen time: 18.779000043869
        !='' time  : 1.2060000896454
        !=='' time : 1.1610000133514
        isset time : 1.1610000133514
Test with short string
        strcmp time: 19.750999927521
        strlen time: 18.803999900818
        !='' time  : 1.4279999732971
        !=='' time : 1.3760001659393
        isset time : 1.4089999198914
Test with long string
        strcmp time: 19.756000041962
        strlen time: 18.770999908447
        !='' time  : 1.4360001087189
        !=='' time : 1.385999917984
        isset time : 1.414999961853

strcmp and strlen are ways slower that isset(..[0]), but checking with !== '' is even faster (and IMHO more readable).

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 21, 2015

Indeed. Checking for empty strings would look nicer if done either via !== '' or empty

@mlocati

This comment has been minimized.

Copy link
Member

mlocati commented Jan 21, 2015

I tend to avoid empty to check if a string is empty (empty('0') == true 😟 ).

@mlocati mlocati referenced this pull request Jan 21, 2015

Merged

Fix #55 and #56 #57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment