search

vinkla / hashids

A small PHP library to generate YouTube-like ids from numbers. Use it when you don't want to expose your database ids to the user.
https://hashids.org/php
MIT License
5.28k stars 418 forks source link

Use separate Bc/Gmp math classes. #106

Closed jwpage closed 6 years ago

jwpage commented 6 years ago

I noticed that the current Hashids\Math class is making a function_exists check with each function call, which was quite slow. I've replaced it with a Hashids\Math\MathFactory that checks if the relevant extension (gmp/bcmath) is loaded instead, which returns a Hashids\Math\MathInterace class.

I've tried to retain backwards compatibility by using the MathFactory inside the (deprecated, no longer used) Hashids\Math class as well.

Here are a couple of blackfire.io profiles for encoding 50,000 hashes.

🚨 A couple of things that need fixing/addressing:

ryan-senn commented 6 years ago

Probably doesn't really matter, but I'd make the methods non-static at that point. Feels odd to have a factory and then not use the created instance.

vinkla commented 6 years ago

Thanks for the pull request @jwpage! It is awesome that you're taking the time to optimise this library.

There are a couple of thing we need to adjust before showing this to @ivanakimov:

We should also note that this change will require a new major release since this is a breaking change.

Probably doesn't really matter, but I'd make the methods non-static at that point. Feels odd to have a factory and then not use the created instance.

I agree, we should make the methods non-static.

I couldn't figure out a way to disable extensions at runtime (either command-line or in PHP), so Hashids\Math\MathFactory is marked as @codeCoverageIgnore. 👎 (Considered adding a bitmask parameter to the constructor to force disabling GMP and/or BC, but is it worth it?)

Hmm, not sure. In what case would we want to enforce this? Lets skip that for now.

tests/Hashids/RuntimeExceptionTest.php also can't be run properly, as there's no way to force disabling extensions at runtime. Could this test be removed instead? 🤷‍♂️

Lets drop the test entirely for now.

jwpage commented 6 years ago

Thanks for the feedback, @vinkla. Hopefully have ticked off all the the issues.

If this is going to be a new major version, I'll go ahead and remove the Hashids\Math class as it's no longer used. I'd also maybe recommend adding @api/@internal docblock tags to indicate what Structural Elements are supported for future backwards compatibility (only Hashids::__construct, Hashids::encode, Hashids::decode?)

vinkla commented 6 years ago

If this is going to be a new major version, I'll go ahead and remove the Hashids\Math class as it's no longer used. I'd also maybe recommend adding @api/@internal docblock tags to indicate what Structural Elements are supported for future backwards compatibility (only Hashids::__construct, Hashids::encode, Hashids::decode?)

Sounds like a good idea.

jwpage commented 6 years ago

Sounds like a good idea.

I might leave @api/@internal tagging for a different 3.0-related PR, as that's not directly related to this PR.

vinkla commented 6 years ago

@ivanakimov now the final decision is up to you. This pull request LGTM. What is your opinion?

jd327 commented 6 years ago

This is great :smile:

@jwpage, I agree with @vinkla -- thanks for taking the time to optimize the code! And @vinkla, thanks for such quick responses and taking the lead :muscle: Feel free to release whenever you're ready!

vinkla commented 6 years ago

Thanks for the pull request @jwpage! I'll bump the branch alias to 3.0 and then we can test the new version a couple of days before releasing it. Let me know if you've any questions!