shdict_get flag comparison #1921

Miniwoffer · 2021-09-23T07:02:11Z

Proposal

Add optional flags argument to shmdict_get(_stale)? that only returns value if flags do not match.
And a 3rd (4th for _stale) return value to indicate if it is a flag match.

Example usage

local object, stale_data, lru_flag = lru:get(key)
local serialized, shm_flag, flag_match = shm:get(key, object and lru_flag)

if flag_match then
    return object
end

if serialized then
    object, err = cjson.decode(serialized)
    if object then
        lru:set(key, object, 0, shm_flag)
    end

    return object, err
end

Why?

Allocating and collecting the memory nedded to return a value can be
very expensive when value is not needed.

Bencmarking

Link to benchmark repo
Read only benchmarking, with random access order.
Tested with 5 000 000 shm lookups on a single worker.

Value 2048 bytes string
  Results: ~75.8% faster

Value 1024 bytes string
  Results: ~46.8% faster

Value 256 bytes string
  Results: ~24.7% faster

Value 16 byte string
  Results: ~19.8% faster

Value number
  Results: ~5.4% faster

Pull requests

stream-lua-nginx Add support for shdict_get flag comparison stream-lua-nginx-module#265
lua-nginx-module Add support for shdict_get flag comparison #1922
lua-resty-core Add support for shdict_get flag comparison lua-resty-core#362

The text was updated successfully, but these errors were encountered:

doujiang24 · 2021-09-24T05:27:15Z

Hi @Miniwoffer
I think we also need to think about if the new API is slower than before when there is no flag used, could you please provide some bench results?

Miniwoffer · 2021-09-24T09:29:45Z

Sure, i had som trouble getting consistent results betwene runs, but managed to get some more or less reliable data.
Benchmarked with numbers, no random order and ran it for 1 800 000 000 gets.

With commits:
728602.810000 std-dev +- 3111.248909

Without commits
727996.340000 std-dev +- 2020.834505

0.083307% +0.149138 -0.149968

Miniwoffer · 2021-10-04T10:04:26Z

Any progress?

zhuizhuhaomeng · 2021-10-04T10:56:06Z

@Miniwoffer, please rebase to the master and change the lua-resty-core in .travis.yml to your branch.

zhuizhuhaomeng · 2021-10-04T10:57:53Z

we need to update the api in doc/HttpLuaModule.wiki.
run ngx-releng in openresty-devel-utils repo, it will convert doc/HttpLuaModule.wiki to README.markdown

Miniwoffer · 2021-10-04T11:40:45Z

@zhuizhuhaomeng Done, travis is green on lua-nginx-module but not lua-resty-core. But master is also red, so i am unsure if its me.

Miniwoffer · 2021-10-04T12:07:34Z

Okay so there are some tests in lua-resty-core that uses get and get_stale with two arguments. Dont know why those tests specify a secound argument, so would need some guidance on how to alter those tests.

Tests in question are t/stdict.t(Test 24 and 25)

Also want me to write tests for lua-rest-core even though the tests in lua-nginx-module should cover everything?

Miniwoffer · 2021-10-04T12:40:01Z

Fixed so Test 24 and 25 in t/shdict.t pass, just changed so key == nil is check first.

zhuizhuhaomeng · 2021-10-04T13:53:28Z

lua-resty-core still failed.
https://app.travis-ci.com/github/openresty/lua-resty-core/builds/239052924

Miniwoffer · 2021-10-04T19:26:58Z

@zhuizhuhaomeng Yeah sorry about that, thought stream was generated from http so didn't implement anything in stream. I ported tho code and the tests over to the stream module.

Miniwoffer · 2021-10-07T07:58:53Z

@zhuizhuhaomeng Think you might have missed the openresty/stream-lua-nginx-module#265 pull request.

Also am i the one who should revert the .travis.yml(cc12437, openresty/stream-lua-nginx-module@e53cdc7, openresty/lua-resty-core@358f770) changes?

Il add a summary of pull requests here and in the original post

Pull requests

stream-lua-nginx Add support for shdict_get flag comparison stream-lua-nginx-module#265
lua-nginx-module Add support for shdict_get flag comparison #1922
lua-resty-core Add support for shdict_get flag comparison lua-resty-core#362

zhuizhuhaomeng · 2021-10-07T12:26:56Z

@Miniwoffer No, you don't need a revert .travis.yml.

zhuizhuhaomeng · 2021-10-07T12:27:37Z

>> Also am i the one who should revert the .travis.yml(cc12437

<cc12437> , ***@***.*** <openresty/stream-lua-nginx-module@e53cdc7> , ***@***.*** <openresty/lua-resty-core@358f770>) changes? Odin Hultgren Van Der Horst ***@***.***> 于2021年10月7日周四下午3:59写道：

…

@zhuizhuhaomeng <https://github.com/zhuizhuhaomeng> Think you might have missed the openresty/stream-lua-nginx-module#265 <openresty/stream-lua-nginx-module#265> pull request. Also am i the one who should revert the .travis.yml(cc12437 <cc12437>, ***@***.*** <openresty/stream-lua-nginx-module@e53cdc7>, ***@***.*** <openresty/lua-resty-core@358f770>) changes? Il add a summary of pull requests here and in the original post Pull requests - stream-lua-nginx openresty/stream-lua-nginx-module#265 <openresty/stream-lua-nginx-module#265> - lua-nginx-module #1922 <#1922> - lua-resty-core openresty/lua-resty-core#362 <openresty/lua-resty-core#362> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1921 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6RHPW5MM74K2SMIK3F7A3UFVHMRANCNFSM5ES7DCGQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

agentzh · 2021-10-07T20:08:20Z

I think it's better to introduce a new Lua method instead of piggybacking the existing get() method so that it won't slow down get().

bjne · 2021-10-08T00:45:26Z

Do you have a suggestion, @agentzh? get_conditional(key, flags) ? perhaps (key, flags, comparator) with comparator defaulting to "~="

or get_if(key, flags, comparator)

agentzh · 2021-10-09T07:34:14Z

@bjne I've found the semantics a bit confusing. I don't see equivalents in redis or memcached's API. So that's probably a bad sign.

What exactly is your use case here? You're using flags as some kind of version number here? We'd better set the semantics straight first?

bjne · 2021-10-09T12:39:31Z

@agentzh We are doing multilevel caching, db -> shm -> lru, but due to lack of (or complexity/performance) of IPC between workers, we are using the lru and shm uint32 flags to store a version of the object (actually just time of shm-insert in ms % 2^32 with a ttl of half that), but it could be any versioning ofcorse.

To keep updates to config available fast, we now get flags and value from shm, and then compare with lru flags and if they are same no need to deserialize.

This is where this idea was born, what performance can be gained from not getting the value at all from shm if we do not need it. And as shown above, quite significant numbers.

The closest thing found in redis I guess is for the CAS (compare-and-swap), dont know if COG (compare-or-get) exists in that context, but that is what we want. Also redis/memcache does not have flags afaik, so this type of op would not make sense there.

At the moment I only see the use for the 'NE' comparator, so I am fine with implementing without a comparator input or eventually with 'NE' as the default.

bjne · 2021-10-11T15:26:07Z

@agentzh ping

agentzh · 2021-10-13T06:41:09Z

Maybe we can add a cas() method here too?

agentzh · 2021-10-13T06:42:36Z

BTW, we also provide lua-resty-jsonb library which does not need any serialization or de-serialization overhead when storing JSON data in shm. But it's not open source. You can contact [email protected] if you're interested.

agentzh · 2021-10-13T06:46:55Z

@bjne BTW, memcached does support flags. that's actually where our flags come from.

bjne · 2021-10-13T07:25:40Z

@agentzh Yeah, we can also implement a cas() method, but perhaps in a separate pull request.
But what do You think about naming of this operation, and where do we go from here?
The fact that we do not need to copy a string to lua (that needs gc) or copy the string at
all is the main performance benefit here.

bjne · 2021-10-14T18:22:35Z

Ping @agentzh

bjne · 2021-10-25T07:52:46Z

So where do we go from here, @agentzh. This is in many cases quite a performance gain, so I hope we can get it merged.

Miniwoffer · 2021-10-28T11:44:00Z

New propossal #1954

This was referenced Sep 23, 2021

Add support for shdict_get flag comparison #1922

Closed

Add support for shdict_get flag comparison openresty/lua-resty-core#362

Closed

Miniwoffer mentioned this issue Oct 4, 2021

Add support for shdict_get flag comparison openresty/stream-lua-nginx-module#265

Closed

Miniwoffer mentioned this issue Oct 27, 2021

CAS and COG operations for shm #1953

Closed

Miniwoffer closed this as completed Oct 28, 2021

bjne mentioned this issue Apr 26, 2024

conditional shm get, eagain #2307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shdict_get flag comparison #1921

shdict_get flag comparison #1921

Miniwoffer commented Sep 23, 2021 •

edited

Loading

doujiang24 commented Sep 24, 2021

Miniwoffer commented Sep 24, 2021 •

edited

Loading

Miniwoffer commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021 •

edited

Loading

Miniwoffer commented Oct 4, 2021

Miniwoffer commented Oct 4, 2021 •

edited

Loading

Miniwoffer commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021

Miniwoffer commented Oct 4, 2021

Miniwoffer commented Oct 7, 2021

zhuizhuhaomeng commented Oct 7, 2021

zhuizhuhaomeng commented Oct 7, 2021 via email

agentzh commented Oct 7, 2021 •

edited

Loading

bjne commented Oct 8, 2021 •

edited

Loading

agentzh commented Oct 9, 2021 •

edited

Loading

bjne commented Oct 9, 2021 •

edited

Loading

bjne commented Oct 11, 2021

agentzh commented Oct 13, 2021

agentzh commented Oct 13, 2021

agentzh commented Oct 13, 2021

bjne commented Oct 13, 2021 •

edited

Loading

bjne commented Oct 14, 2021

bjne commented Oct 25, 2021

Miniwoffer commented Oct 28, 2021

shdict_get flag comparison #1921

shdict_get flag comparison #1921

Comments

Miniwoffer commented Sep 23, 2021 • edited Loading

Proposal

Example usage

Why?

Bencmarking

Pull requests

doujiang24 commented Sep 24, 2021

Miniwoffer commented Sep 24, 2021 • edited Loading

Miniwoffer commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021 • edited Loading

Miniwoffer commented Oct 4, 2021

Miniwoffer commented Oct 4, 2021 • edited Loading

Miniwoffer commented Oct 4, 2021

zhuizhuhaomeng commented Oct 4, 2021

Miniwoffer commented Oct 4, 2021

Miniwoffer commented Oct 7, 2021

Pull requests

zhuizhuhaomeng commented Oct 7, 2021

zhuizhuhaomeng commented Oct 7, 2021 via email

agentzh commented Oct 7, 2021 • edited Loading

bjne commented Oct 8, 2021 • edited Loading

agentzh commented Oct 9, 2021 • edited Loading

bjne commented Oct 9, 2021 • edited Loading

bjne commented Oct 11, 2021

agentzh commented Oct 13, 2021

agentzh commented Oct 13, 2021

agentzh commented Oct 13, 2021

bjne commented Oct 13, 2021 • edited Loading

bjne commented Oct 14, 2021

bjne commented Oct 25, 2021

Miniwoffer commented Oct 28, 2021

Miniwoffer commented Sep 23, 2021 •

edited

Loading

Miniwoffer commented Sep 24, 2021 •

edited

Loading

zhuizhuhaomeng commented Oct 4, 2021 •

edited

Loading

Miniwoffer commented Oct 4, 2021 •

edited

Loading

agentzh commented Oct 7, 2021 •

edited

Loading

bjne commented Oct 8, 2021 •

edited

Loading

agentzh commented Oct 9, 2021 •

edited

Loading

bjne commented Oct 9, 2021 •

edited

Loading

bjne commented Oct 13, 2021 •

edited

Loading