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

'evalSha' is deprecated #49

Closed
ghost opened this issue Nov 14, 2017 · 7 comments
Closed

'evalSha' is deprecated #49

ghost opened this issue Nov 14, 2017 · 7 comments
Assignees

Comments

@ghost
Copy link

ghost commented Nov 14, 2017

@kelunik I think that 763f30a is not in place, I think that a redis client must implement API and not decide what is better, I have some old tested logic that works perfect, but after that commit it fail, my opinion is to use @deprecated ?

@kelunik
Copy link
Member

kelunik commented Nov 14, 2017

@umbri What's the logic you implemented? I guess you can just use eval directly now?

It does implement the API, just not totally low-level.

@ghost
Copy link
Author

ghost commented Nov 14, 2017

@kelunik I understand that this is a feature, but for me I need that old evalSha(), as I say I have some old code that was ported from another language with a lot of tests and using old evalSha() all was fine, now I get error, I understand that I can rewrite it but I think there may be other one as me, I suggest to just mark it as @deprecated and not trigger_error()

@kelunik
Copy link
Member

kelunik commented Nov 14, 2017

If you don't want to get deprecation warnings, you can exclude them in your error reporting setting or explicitly silence your call to evalSha.

We could maybe talk about not deprecating it at all, but given that it newer worked correctly, I don't see a lot of advantage in that.

@ghost
Copy link
Author

ghost commented Nov 14, 2017

I am worry of this extra call for sha1() https://github.com/amphp/redis/blob/master/lib/Redis.php#L1923

I am calling evalSha() very often, 90% off all calls to Redis

@kelunik
Copy link
Member

kelunik commented Nov 14, 2017

We could optimize that and include a lookup cache.

<?php

$cache = [];

function bench(string $script) {
    global $cache;

    // version without cache: return \sha1($script);

    if (isset($cache[$script])) {
        return $cache[$script];
    }

    $sha1 = \sha1($script);
    $cache[$script] = $sha1;

    return $sha1;
}

$start = \microtime(true);

for ($i = 0; $i < 1000000; $i++) {
    bench("foobar = foobar = foobar = foobar = foobar;");
}

var_dump(\microtime(true) - $start);
kelunik@kelunik ❯ ~ ❯ 18:12:25 ❯ 
$ php test.php # with cache
float(0.05865216255188)

kelunik@kelunik ❯ ~ ❯ 18:12:38 ❯ 
$ php test.php # without cache
float(0.33159995079041)

@kelunik kelunik self-assigned this Nov 14, 2017
@ghost
Copy link
Author

ghost commented Nov 17, 2017

#51

@kelunik
Copy link
Member

kelunik commented Nov 17, 2017

Closing, as https://github.com/amphp/redis/releases/tag/v0.3.3 has been released with a script cache.

@kelunik kelunik closed this as completed Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant