refactor: `command()` in Common function #8667

ddevsr · 2024-03-27T08:57:08Z

Description
Improvement.

Benchmark

Test 		Time 	Memory
command_new() 	11.8162 	2 MB
command() 	12.5120 	0 Bytes

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('command_new()', static function () {
            for ($i = 0; $i <= 10; $i++) {
                command_new('env');
            }
        });

        $iterator->add('command()', static function () {
            for ($i=0; $i <= 10; $i++) {
                command('env');
            }
        });


        return $iterator->run(40000);
    }
}

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

ddevsr · 2024-03-27T10:18:49Z

Latest in 427f2a6

Benchmark

Test 		Time 	Memory
command_new() 	11.8162 	2 MB
command() 	12.5120 	0 Bytes

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('command_new()', static function () {
            for ($i = 0; $i <= 10; $i++) {
                command_new('env');
            }
        });

        $iterator->add('command()', static function () {
            for ($i=0; $i <= 10; $i++) {
                command('env');
            }
        });


        return $iterator->run(40000);
    }
}

kenjis · 2024-03-28T02:46:13Z

Benchmarking on macOS shows things are getting very slow.
Why?

Test 		Time 	Memory
command_new() 	0.0203 	0 Bytes
command() 	0.0004 	0 Bytes

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('command_new()', static function () {
            command_new('env');
        });

        $iterator->add('command()', static function () {
            command('env');
        });

        return $iterator->run(10);
    }
}

ddevsr · 2024-03-28T03:12:13Z

I have already benchmark on 2 PC windows, new better then old.

@kenjis I create test simulate multi command running by queue or tasks, what a result when run more iterator or using my test benchmark?

paulbalandan · 2024-03-28T09:54:26Z

Running on macOS with M2, shows similar results with @kenjis

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $outputs = [];
        $template = 'On %s runs:';

        foreach ([10, 1000, 2000, 4000, 40000] as $run) {
            $outputs[] = sprintf($template, number_format($run));

            $iterator = new Iterator();
            $iterator->add('command_new()', static function () {
                command_new('env');
            });
            $iterator->add('command()', static function () {
                command('env');
            });

            $outputs[] = $iterator->run($run);
            $outputs[] = '<br/>';
        }

        return implode("\n", $outputs);
    }
}

On 10 runs:

Test	Time	Memory
command_new()	0.0147	0 Bytes
command()	0.0002	0 Bytes

On 1,000 runs:

Test	Time	Memory
command_new()	0.0166	0 Bytes
command()	0.0170	0 Bytes

On 2,000 runs:

Test	Time	Memory
command_new()	0.0318	0 Bytes
command()	0.0336	0 Bytes

On 4,000 runs:

Test	Time	Memory
command_new()	0.0637	0 Bytes
command()	0.0677	0 Bytes

On 40,000 runs:

Test	Time	Memory
command_new()	0.6371	0 Bytes
command()	0.6644	0 Bytes

I removed the for loop inside the closure as the iterator itself loops the closure already.

paulbalandan · 2024-03-28T10:09:20Z

Running with the loop inside the closure:

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $outputs = [];
        $template = 'On %s runs:';

        foreach ([10, 1000, 2000, 4000, 40000] as $run) {
            $outputs[] = sprintf($template, number_format($run));

            $iterator = new Iterator();
            $iterator->add('command_new()', static function () {
                for ($i = 0; $i <= 10; $i++) {
                    command_new('env');
                }
            });
            $iterator->add('command()', static function () {
                for ($i = 0; $i <= 10; $i++) {
                    command('env');
                }
            });

            $outputs[] = $iterator->run($run);
            $outputs[] = '<br/>';
        }

        return implode("\n", $outputs);
    }
}

On 10 runs:

Test	Time	Memory
command_new()	0.0176	0 Bytes
command()	0.0020	0 Bytes

On 1,000 runs:

Test	Time	Memory
command_new()	0.1605	0 Bytes
command()	0.1714	0 Bytes

On 2,000 runs:

Test	Time	Memory
command_new()	0.3197	0 Bytes
command()	0.3409	0 Bytes

On 4,000 runs:

Test	Time	Memory
command_new()	0.6282	0 Bytes
command()	0.6774	0 Bytes

On 40,000 runs:

Test	Time	Memory
command_new()	6.2965	0 Bytes
command()	6.7474	0 Bytes

kenjis · 2024-03-28T11:15:56Z

When the iteration is very small, command() is much, much faster. Why?

kenjis · 2024-03-29T00:18:25Z

If I quit almost all applications except terminal, and disables Xdebug.

Test	Time	Memory
command_new()	0.0045	0 Bytes
command()	0.0040	0 Bytes

But if I am running Firefox and PhpStorm, the values sometime differ a lot like:

Test	Time	Memory
command_new()	0.0059	0 Bytes
command()	0.0293	0 Bytes

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('command_new()', static function () {
        });

        $iterator->add('command()', static function () {
        });

        return $iterator->run(40000);
    }
}

kenjis · 2024-03-29T00:33:42Z

Okay, it seems about 20% faster on my MBA.

However, there is little tests of this parsing process, so close scrutiny is required to ensure that the new implementation is correct.

Test	Time	Memory
command_new()	0.2412	0 Bytes
command()	0.2985	0 Bytes

<?php

namespace App\Controllers;

use CodeIgniter\Debug\Iterator;
use InvalidArgumentException;

class Home extends BaseController
{
    public function index(): string
    {
        $iterator = new Iterator();

        $iterator->add('command_new()', static function () {
            Home::command_new('env');
        });

        $iterator->add('command()', static function () {
            Home::command('env');
        });

        return $iterator->run(400000);
    }

    public static function command_new(string $command)
    {
        $regexString = '([^\s]+?)(?:\s|$)';
        $regexQuoted = '"([^"\\\\]*(?:\\\\.[^"\\\\]*)*)"|\'([^\'\\\\]*(?:\\\\.[^\'\\\\]*)*)\'';

        // Match all arguments at once
        preg_match_all('/' . $regexQuoted . '|' . $regexString . '/', $command, $matches, PREG_SET_ORDER);

        $args = [];

        foreach ($matches as $match) {
            // Determine which part of the match is the actual argument
            $arg    = $match[1] !== '' ? stripcslashes($match[1]) : (isset($match[3]) ? stripcslashes($match[3]) : stripcslashes($match[2]));
            $args[] = $arg;
        }

        $command     = array_shift($args);
        $params      = [];
        $optionValue = false;

        foreach ($args as $i => $arg) {
            if (mb_strpos($arg, '-') !== 0) {
                if ($optionValue) {
                    // if this was an option value, it was already
                    // included in the previous iteration
                    $optionValue = false;
                } else {
                    // add to segments if not starting with '-'
                    // and not an option value
                    $params[] = $arg;
                }

                continue;
            }

            $arg   = ltrim($arg, '-');
            $value = null;

            if (isset($args[$i + 1]) && mb_strpos($args[$i + 1], '-') !== 0) {
                $value       = $args[$i + 1];
                $optionValue = true;
            }

            $params[$arg] = $value;
        }
    }

    public static function command(string $command)
    {
        $regexString = '([^\s]+?)(?:\s|(?<!\\\\)"|(?<!\\\\)\'|$)';
        $regexQuoted = '(?:"([^"\\\\]*(?:\\\\.[^"\\\\]*)*)"|\'([^\'\\\\]*(?:\\\\.[^\'\\\\]*)*)\')';

        $args   = [];
        $length = strlen($command);
        $cursor = 0;

        /**
         * Adopted from Symfony's `StringInput::tokenize()` with few changes.
         *
         * @see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Input/StringInput.php
         */
        while ($cursor < $length) {
            if (preg_match('/\s+/A', $command, $match, 0, $cursor)) {
                // nothing to do
            } elseif (preg_match('/' . $regexQuoted . '/A', $command, $match, 0, $cursor)) {
                $args[] = stripcslashes(substr($match[0], 1, strlen($match[0]) - 2));
            } elseif (preg_match('/' . $regexString . '/A', $command, $match, 0, $cursor)) {
                $args[] = stripcslashes($match[1]);
            } else {
                // @codeCoverageIgnoreStart
                throw new InvalidArgumentException(sprintf(
                    'Unable to parse input near "... %s ...".',
                    substr($command, $cursor, 10)
                ));
                // @codeCoverageIgnoreEnd
            }

            $cursor += strlen($match[0]);
        }

        $command     = array_shift($args);
        $params      = [];
        $optionValue = false;

        foreach ($args as $i => $arg) {
            if (mb_strpos($arg, '-') !== 0) {
                if ($optionValue) {
                    // if this was an option value, it was already
                    // included in the previous iteration
                    $optionValue = false;
                } else {
                    // add to segments if not starting with '-'
                    // and not an option value
                    $params[] = $arg;
                }

                continue;
            }

            $arg   = ltrim($arg, '-');
            $value = null;

            if (isset($args[$i + 1]) && mb_strpos($args[$i + 1], '-') !== 0) {
                $value       = $args[$i + 1];
                $optionValue = true;
            }

            $params[$arg] = $value;
        }
    }
}

$ php -v
PHP 8.2.16 (cli) (built: Feb 16 2024 05:30:16) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.16, Copyright (c) Zend Technologies
$ sw_vers 
ProductName:	macOS
ProductVersion:	12.7.3
BuildVersion:	21H1015

kenjis · 2024-03-29T00:40:00Z

However, such micro-optimization would have little or no effect on performance improvement in the real world.

I cannot assume that command() will be executed many times in a request.

refactor: command() in Common function

bee24d3

ddevsr changed the title ~~refactor: command() in Common function~~ refactor: command() in Common function Mar 27, 2024

kenjis added the refactor Pull requests that refactor code label Mar 27, 2024

ddevsr added 2 commits March 27, 2024 16:30

fix: regex pattern

a481c94

refactor: try efficient

427f2a6

ddevsr marked this pull request as ready for review March 27, 2024 10:25

ddevsr requested review from paulbalandan and kenjis March 27, 2024 10:27

kenjis added the tests needed Pull requests that need tests label Mar 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `command()` in Common function #8667

refactor: `command()` in Common function #8667

ddevsr commented Mar 27, 2024 •

edited

ddevsr commented Mar 27, 2024 •

edited

kenjis commented Mar 28, 2024

ddevsr commented Mar 28, 2024 •

edited

paulbalandan commented Mar 28, 2024 •

edited

paulbalandan commented Mar 28, 2024 •

edited

kenjis commented Mar 28, 2024

kenjis commented Mar 29, 2024 •

edited

kenjis commented Mar 29, 2024

kenjis commented Mar 29, 2024

refactor: command() in Common function #8667

Are you sure you want to change the base?

refactor: command() in Common function #8667

Conversation

ddevsr commented Mar 27, 2024 • edited

ddevsr commented Mar 27, 2024 • edited

kenjis commented Mar 28, 2024

ddevsr commented Mar 28, 2024 • edited

paulbalandan commented Mar 28, 2024 • edited

paulbalandan commented Mar 28, 2024 • edited

kenjis commented Mar 28, 2024

kenjis commented Mar 29, 2024 • edited

kenjis commented Mar 29, 2024

kenjis commented Mar 29, 2024

refactor: `command()` in Common function #8667

refactor: `command()` in Common function #8667

ddevsr commented Mar 27, 2024 •

edited

ddevsr commented Mar 27, 2024 •

edited

ddevsr commented Mar 28, 2024 •

edited

paulbalandan commented Mar 28, 2024 •

edited

paulbalandan commented Mar 28, 2024 •

edited

kenjis commented Mar 29, 2024 •

edited