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

Add separate checks for all polyfilled functions and constants #252

Merged
merged 1 commit into from
May 8, 2020

Conversation

JanJakes
Copy link
Contributor

As originally discussed in symfony/polyfill-mbstring#6 and later in #251 in some scenarios it may be better to check for each constant/function before defining it. Such scenarios are typically those where some project or a library defines its polyfills for some parts of extension functionality but not all of it.

Probably the most popular example is WordPress, defining some polyfills in wp-includes/compat.php but not including complete functionalities (i.e. it defines only a few mb_ functions). Plugin developers then may need other mb_ functions and need to load those that WordPress didn't.

There may be some performance concerns but it seems that checks such as function_exists are very fast - in fact, PHP tries to evaluate both function_exists and defined at compile time (see also StackOverflow). This means on installs that do have the polyfilled extensions installed, there should be zero overhead (the conditions will evaluate to true at compile time).

Note that many of the currently existing polyfills already perform checks for each function separately (most of the PHP-version polyfills).

Closes #251.

@nicolas-grekas
Copy link
Member

To validate the change, can you please run a simple benchmark with nothing else but a require for vendor/autoload.php, in three situations:

  • all extensions loaded in php, polyfills installed
  • no extensions loaded in php, polyfills installed
  • again but with NO polyfills installed
    ?

If you think about other relevant benchmarks, please proceed :) Thanks.

@capuderg
Copy link

capuderg commented May 5, 2020

I've run some basic time benchmarks. I've only tested the mbstrings polyfill library (not this whole library)

A new PHP 7.4.5 CLI docker container was used as an environment.

I've run the basic require_once __DIR__ . '/vendor/autoload.php'; 1000 times and here are the averages (in seconds) for each situation:

  • "empty" composer (no packages required) with mbstring extension disabled: 0.0179128654

  • "empty" composer (no packages required) with mbstring extension enabled: 0.01777232742

  • "symfony/polyfill-mbstring = 1.15" required with mbstring extension disabled: 0.02518279123

  • "symfony/polyfill-mbstring = 1.15" required with mbstring extension enabled: 0.02492605114

  • "symfony/polyfill-mbstring with updated bootstrap.php" required with mbstring extension disabled: 0.02665189552

  • "symfony/polyfill-mbstring with updated bootstrap.php" required with mbstring extension enabled: 0.02570387244

I hope this helps :)

@JanJakes
Copy link
Contributor Author

JanJakes commented May 7, 2020

@capuderg Great, thanks! I didn't have time to get to this yet, only started to prepare a PHP Docker container without mbstring. This looks good, not sure if we need any more tests.

@nicolas-grekas Do you have any clues why the builds are failing? The Parse error: syntax error, unexpected T_OBJECT_OPERATOR in C:\projects\polyfill\vendor\bin\.phpunit\phpunit-4.8\phpunit on line 13 doesn't say much.

@nicolas-grekas nicolas-grekas force-pushed the separate-symbol-checks branch from dbaa734 to ac8583e Compare May 8, 2020 16:44
@nicolas-grekas nicolas-grekas force-pushed the separate-symbol-checks branch from ac8583e to a00cffe Compare May 8, 2020 16:50
@nicolas-grekas
Copy link
Member

Thank you @JanJakes.

@nicolas-grekas nicolas-grekas merged commit c140f6a into symfony:master May 8, 2020
@nicolas-grekas
Copy link
Member

And thank you @capuderg also.

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