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

Feature/shdict zset #1272

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ZigzagAK
Copy link

@ZigzagAK ZigzagAK commented Mar 8, 2018

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

Hello!
May be interresting DICT.zset feature (but without ffi)?

@ZigzagAK ZigzagAK changed the title 7Feature/shdict zset Feature/shdict zset Mar 8, 2018
@spacewander
Copy link
Member

Thank you for your pull request.
But there are some problems I have to point out:

  1. We are trying hard to migrate Lua C API to FFI, so adding more non-FFI API is not desiring. Unless there is not FFI infrastructure like cosocket stuff, but shdict is not. Not even mentioning that the current shdict's Lua C API implementation is unsafe, discussed in Deadlock in ngx_http_lua_shdict_set_helper #1207.
  2. Implement zset with doubly linked list doesn't work well when people need to do zrange (zrange is not implemented yet, but we need to do it one day if we add zset to shdict type). Redis implements zset with skip list.

@spacewander
Copy link
Member

@ZigzagAK
Apologize for my mistake. The zset is actually built on top of rbtree, so my second problem doesn't make sense at all.

@ZigzagAK
Copy link
Author

ZigzagAK commented Mar 9, 2018

@spacewander
I don't use ffi (lua-resty-core) api. In my use case i can't found performance improvements with FFI based api. Integral performance of my application with FFI looks like a performance with standart LUA C API (luajit).

I checked my application with https://github.com/openresty/stapxx#lj-vm-states and https://github.com/openresty/stapxx#ngx-lj-trace-exits. When i use FFI api, application spend all the time in JIT code, but performance was not greather when i used standart LUA C API (luajit).

@spacewander
Copy link
Member

@ZigzagAK
Maybe the bottleneck of the application is not JIT.
Besides the performance, the shdict API implemented in Lua C API is unsafe. They hold shm lock when calling the Lua C API. When LuaJIT runs the Lua C API, it may do GC check and trigger the GC. If the shdict is accessed again via the __gc metamethod, there will be a deadlock.

@ZigzagAK
Copy link
Author

ZigzagAK commented Mar 10, 2018

@spacewander

If the shdict is accessed again via the __gc metamethod, there will be a deadlock.

Yes, I understad it. Workaround: don't use lua objects with __gc metamethods, which may call shdict methods on the same shm.

Maybe the bottleneck of the application is not JIT.

Bootleneck was in the CPU. Most of the time CPU spent in strings and tables allocation/deallocation, shm mutex concurency. But in that moment i couldn't decrease strings and tables usage. And performance without ffi is acceptable and increase it on 10-20% is not nessessary. The performance on 6 core CPU is around 15k/sec with all lua code. We can simple increase number of virtual nodes.

@mergify
Copy link

mergify bot commented Jun 26, 2020

This pull request is now in conflict :(

@mergify
Copy link

mergify bot commented Mar 20, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 20, 2023
@mergify mergify bot removed the conflict label May 10, 2023
@mergify
Copy link

mergify bot commented May 10, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 10, 2023
@mergify mergify bot removed the conflict label Sep 23, 2023
@mergify
Copy link

mergify bot commented Sep 23, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Sep 23, 2023
@mergify mergify bot removed the conflict label Mar 6, 2024
Copy link

mergify bot commented Mar 6, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 6, 2024
@mergify mergify bot removed the conflict label May 27, 2024
Copy link

mergify bot commented May 27, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants