Skip to content

Adds memory information about the scripts' cache to INFO #4883

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.

Sign up for GitHub

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

Jump to bottom
Merged
antirez merged 2 commits into from
Jul 23, 2018
Merged

Adds memory information about the scripts' cache to INFO #4883

antirez merged 2 commits into from
Jul 23, 2018

Conversation

@itamarhaber
Copy link
Member

@itamarhaber itamarhaber commented Apr 30, 2018

Implementation notes: as INFO is "already broken", I didn't want to break it further. Instead of computing the server.lua_script dict size on every call, I'm keeping a running sum of the body's length and dict overheads.

This implementation is naive as it does not take into consideration dict rehashing, but that inaccuracy pays off in speed ;)

Demo time:

$ redis-cli info memory | grep "script"
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "" 0 ; redis-cli info memory | grep "script"
(nil)
used_memory_scripts:120
used_memory_scripts_human:120B
number_of_cached_scripts:1
$ redis-cli script flush ; redis-cli info memory | grep "script"
OK
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "return('Hello, Script Cache :)')" 0 ; redis-cli info memory | grep "script"
"Hello, Script Cache :)"
used_memory_scripts:152
used_memory_scripts_human:152B
number_of_cached_scripts:1
$ redis-cli eval "return redis.sha1hex(\"return('Hello, Script Cache :)')\")" 0 ; redis-cli info memory | grep "script"
"1be72729d43da5114929c1260a749073732dc822"
used_memory_scripts:232
used_memory_scripts_human:232B
number_of_cached_scripts:2
✔ 19:03:54 redis [lua_scripts-in-info-memory L ✚…⚑] $ redis-cli evalsha 1be72729d43da5114929c1260a749073732dc822 0
"Hello, Script Cache :)"

@itamarhaber
Adds memory information about the script's cache to INFO
49890c8 
Implementation notes: as INFO is "already broken", I didn't want to break it further. Instead of computing the server.lua_script dict size on every call, I'm keeping a running sum of the body's length and dict overheads.

This implementation is naive as it **does not** take into consideration dict rehashing, but that inaccuracy pays off in speed ;)

Demo time:

```bash
$ redis-cli info memory | grep "script"
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "" 0 ; redis-cli info memory | grep "script"
(nil)
used_memory_scripts:120
used_memory_scripts_human:120B
number_of_cached_scripts:1
$ redis-cli script flush ; redis-cli info memory | grep "script"
OK
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "return('Hello, Script Cache :)')" 0 ; redis-cli info memory | grep "script"
"Hello, Script Cache :)"
used_memory_scripts:152
used_memory_scripts_human:152B
number_of_cached_scripts:1
$ redis-cli eval "return redis.sha1hex(\"return('Hello, Script Cache :)')\")" 0 ; redis-cli info memory | grep "script"
"1be72729d43da5114929c1260a749073732dc822"
used_memory_scripts:232
used_memory_scripts_human:232B
number_of_cached_scripts:2
✔ 19:03:54 redis [lua_scripts-in-info-memory L ✚…⚑] $ redis-cli evalsha 1be72729d43da5114929c1260a749073732dc822 0
"Hello, Script Cache :)"
```
@itamarhaber itamarhaber changed the title Adds memory information about the script's cache to INFO Adds memory information about the scripts' cache to INFO Apr 30, 2018
@itamarhaber
Copy link
Member Author

itamarhaber commented Jun 9, 2018

TODO: once merged, this needs to be added to INFO's documentation

@itamarhaber itamarhaber mentioned this pull request Jul 22, 2018
Closed
@oranagra
Copy link
Member

oranagra commented Jul 22, 2018 •
edited
Loading

@itamarhaber, very good and needed improvement.
personally i would suggest to count the script body using sdsAllocSize or better yet, sdsZmallocSize rather than sdslen, and also think you should count the script name.

Other than that, i think the scripts memory metric should include the following:

  1. the hash table itself: dictSlots(server.lua_scripts) * sizeof(dictEntry*)
  2. the repl_scriptcache_fifo, note that all the values in that list are the same length, so there's actually not need to iterate on the list, so something like:
dictSize(server.repl_scriptcache_dict) * sizeof(dictEntry) +
dictSlots(server.repl_scriptcache_dict) * sizeof(dictEntry*) +
listLength(server.repl_scriptcache_fifo) * sizeof(listNode) + 
listLength(server.repl_scriptcache_fifo) ? (listLength(server.repl_scriptcache_fifo) * sdsZmallocSize(listNodeValue(listFirst(server.repl_scriptcache_fifo)))) : 0

i think you should sum your server.lua_scripts_mem and the bullets i just suggested in getMemoryOverheadData, and that metric should be used in both INFO and MEMORY commands.

@itamarhaber
Copy link
Member Author

itamarhaber commented Jul 22, 2018

@oranagra thanks for the review!

  1. Using sdsZmallocSize now
  2. 1+2 done
  3. Included 1+2 in MEMORY STATS, but not in INFO - reasoning: overheads in the former, real data in the latter.
  4. Added a MEMORY DOCTOR advice (currently very naive, was tempted to compute the Hemming/Levinstein distances between scripts)
  5. Corrected "LUA" to "Lua"

Note: someone needs to fix "MEMORY HELP" to follow the "standard" help mechanism.

@antirez
Copy link
Contributor

antirez commented Jul 23, 2018

@itamarhaber, @oranagra Thanks, looks good but I'm going to change 42 ;-)

@antirez antirez merged commit 445a2a2 into redis:unstable Jul 23, 2018
@antirez
Copy link
Contributor

antirez commented Jul 23, 2018

Merged, thank you.

antirez added a commit that referenced this pull request Jul 23, 2018
@antirez
Change 42 to 1000 as warning level for cached scripts.
2c07c10 
Related to #4883.
antirez added a commit that referenced this pull request Jul 23, 2018
@antirez
Change 42 to 1000 as warning level for cached scripts.
9a375e5 
Related to #4883.
itamarhaber referenced this pull request in itamarhaber/redis-doc Jul 24, 2018
@itamarhaber
Adds Lua metrics
93f730a 
As per https://github.com/antirez/redis/pull/4883

Signed-off-by: Itamar Haber <itamar@redislabs.com>
@itamarhaber itamarhaber mentioned this pull request Jul 24, 2018
Closed
itamarhaber referenced this pull request in itamarhaber/redis-doc Jul 24, 2018
@itamarhaber
Adds Lua metrics to INFO and MEMORY STATS
931fb9e 
As per https://github.com/antirez/redis/pull/4883

Signed-off-by: Itamar Haber <itamar@redislabs.com>
@itamarhaber itamarhaber mentioned this pull request Jul 24, 2018
Merged
@itamarhaber
Copy link
Member Author

itamarhaber commented Jul 24, 2018

Yippie :)

  1. Oran noted that INFO would also benefit from adding the overhead metric - after initially not agreeing, on second thought, I tend to agree. A new PR?
  2. Added to the documentation via Adds Lua metrics to INFO and MEMORY STATS redis-doc#946

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Dec 17, 2018
@antirez
Merge pull request redis#4883 from itamarhaber/lua_scripts-in-info-me…
3360c04 
…mory

Adds memory information about the scripts' cache to INFO
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Dec 17, 2018
@antirez
Change 42 to 1000 as warning level for cached scripts.
12665ba 
Related to redis#4883.
itamarhaber referenced this pull request in redis/redis-doc Oct 14, 2019
@itamarhaber
Adds Lua metrics to INFO and MEMORY STATS (#947)
5ca4686 
As per https://github.com/antirez/redis/pull/4883

Signed-off-by: Itamar Haber <itamar@redislabs.com>
pulllock pushed a commit to pulllock/redis that referenced this pull request Jun 28, 2023
@antirez
Change 42 to 1000 as warning level for cached scripts.
3dc0f1d 
Related to redis#4883.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@itamarhaber @oranagra @antirez