You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/06/29 14:34:26 UTC
[GitHub] [incubator-kvrocks] xiaobiaozhao opened a new pull request, #697: ✨ feat: luajit replace lua
xiaobiaozhao opened a new pull request, #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697
1. Luajit replace lua
2. rm test case "cmsgpack can pack and unpack circular references". Becase luajit's core code diffrent from lua. Also, different compilers produce different code execution effects (GCC&clang on Ubuntu).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910670637
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
I got it. It is due to the element order of a table is unstable. This reminds me of Map in golang.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910680541
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
Yes， It behaves differently in different compilers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915022742
##########
CMakeLists.txt:
##########
@@ -25,8 +25,9 @@ option(ENABLE_ASAN "enable address santinizer" OFF)
option(ENABLE_TSAN "enable thread santinizer" OFF)
option(ASAN_WITH_LSAN "enable leak santinizer while address santinizer is enabled" ON)
option(ENABLE_STATIC_LIBSTDCXX "link kvrocks with static library of libstd++ instead of shared library" ON)
+option(USE_LUAJIT "enable luajit replace lua" ON)
Review Comment:
```suggestion
option(USE_LUAJIT "use luaJIT instead of lua" ON)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910596088
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
Why there are two different values of `h`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911657229
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
> I think this header does not solve the issue in #609, and we need some investigation to determine how to avoid memory leaks in c++ in luajit.
@xiaobiaozhao you can try to call `DestroyState` in the Server destructor since it leaks by `cjson` open would init the buffer(1024). But I'm wondering why it happens in luajit only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911635757
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
Thanks, seems there is a detail comment: https://github.com/luajit/luajit/blob/master/src/lj_err.c#L20-L67
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r912035089
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
I can't reproduce in my ubuntu 20.4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1190171329
Thanks for @xiaobiaozhao great contribution again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1190166771
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910682245
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
Review Comment:
Yes, I will checkout this personal repo to KvrocksLabs org
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1183428776
> There are some memory leaks reported by ASan which are steadily reproduced and need some investigation.
>
> The rest is good for me. 👍
Those memory leaks looks not related to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910672053
##########
CMakeLists.txt:
##########
@@ -59,15 +59,15 @@ include(cmake/snappy.cmake)
include(cmake/lz4.cmake)
include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
-include(cmake/lua.cmake)
+include(cmake/luajit.cmake)
Review Comment:
I think we can provide a build option to switch between luajit and lua for progressive substitution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1173080620
Many thanks for @xiaobiaozhao hard try
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r916552541
##########
.github/workflows/kvrocks.yaml:
##########
@@ -101,6 +101,9 @@ jobs:
os: ubuntu-18.04
with_ninja: --ninja
without_jemalloc: -DDISABLE_JEMALLOC=ON
+ - name: Ubuntu GCC without luajit
+ os: ubuntu-18.04
+ without_luajit: -DUSE_LUAJIT=OFF
Review Comment:
I think you need to add `${{ matrix.without_luajit }}` to line 140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1176027782
@tisonkun and @ShooterIT can take a look if you have time. For https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910672053, I also prefer providing an option to let user fallback if they have any problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910580208
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -235,37 +235,37 @@ start_server {tags {"scripting"}} {
return h
} 0
} {d3ffffff0000000000}
- test {EVAL - cmsgpack can pack and unpack circular references?} {
- r eval {local a = {x=nil,y=5}
- local b = {x=a}
- a['x'] = b
- local encoded = cmsgpack.pack(a)
- local h = ""
- -- cmsgpack encodes to a depth of 16, but can't encode
- -- references, so the encoded object has a deep copy recursive
- -- depth of 16.
- for i = 1, #encoded do
- h = h .. string.format("%02x",string.byte(encoded,i))
- end
- -- when unpacked, re.x.x != re because the unpack creates
- -- individual tables down to a depth of 16.
- -- (that's why the encoded output is so large)
- local re = cmsgpack.unpack(encoded)
- assert(re)
- assert(re.x)
- assert(re.x.x.y == re.y)
- assert(re.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- maximum working depth:
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- now the last x would be b above and has no y
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
- -- so, the final x.x is at the depth limit and was assigned nil
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
- } 0
- } {82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0 1 1}
+ #test {EVAL - cmsgpack can pack and unpack circular references?} {
+ # r eval {local a = {x=nil,y=5}
Review Comment:
I had fix this test. The case is from here https://github.com/antirez/lua-cmsgpack/blob/master/test.lua#L393
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910671915
##########
CMakeLists.txt:
##########
@@ -59,15 +59,15 @@ include(cmake/snappy.cmake)
include(cmake/lz4.cmake)
include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
-include(cmake/lua.cmake)
+include(cmake/luajit.cmake)
Review Comment:
I think we can provide a build option to switch from luajit and lua for progressive substitution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911120337
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
+ GIT_TAG 803487f8b01c672495a2fcd29dcbed09e4fd6319
+)
Review Comment:
done https://github.com/apache/incubator-kvrocks/pull/697/commits/33e874e0e4acc93dac2f7a9b4d5bdf9cf56fe37e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r912032493
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
The issus still exists. https://github.com/apache/incubator-kvrocks/pull/697/commits/707b177d60daf37dfcfbdc75a9a62ea4238a966d
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1175852854
> [here](https://github.com/redis/redis/commits/unstable/deps/lua/src) The part of the lua code is modified for Redis 7.0, mainly the "redis function". I don't think this part needs to be ported to KVROCKS at this time
Thanks for your investigation.
How do you think about this idea: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910672053 ?
I think it is not a hard job to support both since all these APIs are same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1184070099
> Those memory leaks looks not related to this PR.
Seems these reported leaks are steadily reproduced for more than 10 times, and do not appear in other PRs and the unstable branch, so I think it is hard to say that it is not related to this PR.
I'll take a deeper look when I have time : )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911055245
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
Review Comment:
I pushed your commits, you can just switch to the new repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1170131269
> rm test case "cmsgpack can pack and unpack circular references". Becase luajit's core code diffrent from lua. Also, different compilers produce different code execution effects (GCC&clang on Ubuntu).
I did not get this point, could you elaborate on it?
I think that the JIT mechanism should not affect the original semantics, and the same interpreter / JIT program should keep same behavior while compiled by different C compilers. So we should consider it as a serious problem if it really has some difference on semantics between the original interpreter and the JIT, or between different C compilers.
And I think the same problem as #609 (#614) for luajit need to be solved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1188788688
> > Those memory leaks looks not related to this PR.
>
> Seems these reported leaks are steadily (100%) reproduced for more than 10 times, and do not appear in other PRs and the unstable branch, so I think it is hard to say that it is not related to this PR.
>
> I'll take a deeper look when I have time : )
I try to fix those issues in #714, looks cases like `CommandFetchFile` and `CommandBPop` are disappear, but still got a new leak warning and I'm investigating it. Guys can also have a look if you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1190113789
@tisonkun @PragmaTwice @ShooterIT I think we can merge this PR if no objections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r916552541
##########
.github/workflows/kvrocks.yaml:
##########
@@ -101,6 +101,9 @@ jobs:
os: ubuntu-18.04
with_ninja: --ninja
without_jemalloc: -DDISABLE_JEMALLOC=ON
+ - name: Ubuntu GCC without luajit
+ os: ubuntu-18.04
+ without_luajit: -DUSE_LUAJIT=OFF
Review Comment:
I think you need add ${{ matrix.without_luajit }} to line 140
##########
.github/workflows/kvrocks.yaml:
##########
@@ -101,6 +101,9 @@ jobs:
os: ubuntu-18.04
with_ninja: --ninja
without_jemalloc: -DDISABLE_JEMALLOC=ON
+ - name: Ubuntu GCC without luajit
+ os: ubuntu-18.04
+ without_luajit: -DUSE_LUAJIT=OFF
Review Comment:
I think you need add `${{ matrix.without_luajit }}` to line 140
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r917207736
##########
.github/workflows/kvrocks.yaml:
##########
@@ -101,6 +101,9 @@ jobs:
os: ubuntu-18.04
with_ninja: --ninja
without_jemalloc: -DDISABLE_JEMALLOC=ON
+ - name: Ubuntu GCC without luajit
+ os: ubuntu-18.04
+ without_luajit: -DUSE_LUAJIT=OFF
Review Comment:
done https://github.com/apache/incubator-kvrocks/pull/697/commits/d4a6ff9a7a0a38d01f5ea0228714c1cd0d98f644
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910538941
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -235,37 +235,37 @@ start_server {tags {"scripting"}} {
return h
} 0
} {d3ffffff0000000000}
- test {EVAL - cmsgpack can pack and unpack circular references?} {
- r eval {local a = {x=nil,y=5}
- local b = {x=a}
- a['x'] = b
- local encoded = cmsgpack.pack(a)
- local h = ""
- -- cmsgpack encodes to a depth of 16, but can't encode
- -- references, so the encoded object has a deep copy recursive
- -- depth of 16.
- for i = 1, #encoded do
- h = h .. string.format("%02x",string.byte(encoded,i))
- end
- -- when unpacked, re.x.x != re because the unpack creates
- -- individual tables down to a depth of 16.
- -- (that's why the encoded output is so large)
- local re = cmsgpack.unpack(encoded)
- assert(re)
- assert(re.x)
- assert(re.x.x.y == re.y)
- assert(re.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- maximum working depth:
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- now the last x would be b above and has no y
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
- -- so, the final x.x is at the depth limit and was assigned nil
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
- } 0
- } {82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0 1 1}
+ #test {EVAL - cmsgpack can pack and unpack circular references?} {
+ # r eval {local a = {x=nil,y=5}
Review Comment:
Why we disable this test case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910596088
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
Why there are two different value of `h`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910673049
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
+ GIT_TAG 803487f8b01c672495a2fcd29dcbed09e4fd6319
+)
+
+FetchContent_GetProperties(luajit)
+if(NOT lua_POPULATED)
+ FetchContent_Populate(luajit)
+
+ set(LUA_CFLAGS "-DLUA_ANSI -DENABLE_CJSON_GLOBAL -DREDIS_STATIC= -DLUA_USE_MKSTEMP")
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
+ set(LUA_CFLAGS "${LUA_CFLAGS} -isysroot ${CMAKE_OSX_SYSROOT}")
+ endif()
+
+ if (CMAKE_HOST_APPLE)
+ add_custom_target(make_luajit COMMAND make libluajit.a
+ "CFLAGS=${LUA_CFLAGS}"
+ MACOSX_DEPLOYMENT_TARGET=11.0
+ WORKING_DIRECTORY ${luajit_SOURCE_DIR}/src
+ BYPRODUCTS ${luajit_SOURCE_DIR}/src/libluajit.a
+ )
+ else()
+ add_custom_target(make_luajit COMMAND make libluajit.a
+ "CFLAGS=${LUA_CFLAGS}"
+ WORKING_DIRECTORY ${luajit_SOURCE_DIR}/src
+ BYPRODUCTS ${luajit_SOURCE_DIR}/src/libluajit.a
+ )
+ endif()
Review Comment:
You can introduce a variable to avoid duplication.
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
I think this header does not solve the issue in #609, and we need some investigation to determine how to avoid memory leaks in c++ in luajit.
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
Review Comment:
I think we could need to build a new repo under some organization rather than a personal one.
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
+ GIT_TAG 803487f8b01c672495a2fcd29dcbed09e4fd6319
+)
Review Comment:
Now we have `FetchContent_DeclareGitHubWithMirror`, refer to https://github.com/apache/incubator-kvrocks/blob/unstable/cmake/lua.cmake
##########
CMakeLists.txt:
##########
@@ -59,15 +59,15 @@ include(cmake/snappy.cmake)
include(cmake/lz4.cmake)
include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
-include(cmake/lua.cmake)
+include(cmake/luajit.cmake)
Review Comment:
I think we can provide a build option to switch from luajit and lua for progressive substitution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911112281
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
Review Comment:
done https://github.com/apache/incubator-kvrocks/pull/697/commits/c4a66978d9e3c313a186ac577a98ee2a635ca8e5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911045780
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
Review Comment:
@xiaobiaozhao Can you help to submit your changes to https://github.com/KvrocksLabs/LuaJIT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1183404899
There are some memory leaks reported by ASan which are steadily reproduced and need some investigation.
The rest is good for me. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915025827
##########
src/server.h:
##########
@@ -30,7 +30,13 @@
#include <memory>
#include <unordered_map>
+#ifdef USE_LUAJIT
+#include <lua.hpp>
+#else
+#include <lauxlib.h>
#include <lua.h>
+#include <lualib.h>
+#endif
Review Comment:
With https://github.com/KvrocksLabs/lua/pull/4, we do not need `USE_LUAJIT` definition checking. Could you help to review it? @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r912483772
##########
src/server.cc:
##########
@@ -174,6 +174,7 @@ Status Server::Start() {
void Server::Stop() {
stop_ = true;
+ Lua::DestroyState(lua_);
Review Comment:
Can move this line to the server destructor and delete the server after the server threads join.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915962350
##########
CMakeLists.txt:
##########
@@ -59,15 +59,15 @@ include(cmake/snappy.cmake)
include(cmake/lz4.cmake)
include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)
-include(cmake/lua.cmake)
+include(cmake/luajit.cmake)
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1179547117
Maybe you need to add `*.hpp` to line 42 in lua.cmake to pass the "Ubuntu GCC without luajit" job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1172850854
In order to use sanitizers, you not only need to add compile options, but also link options. (which is why CI is currently failing)
Also, don't forget to remove these sanitizer options when the bug is fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910581317
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -235,37 +235,37 @@ start_server {tags {"scripting"}} {
return h
} 0
} {d3ffffff0000000000}
- test {EVAL - cmsgpack can pack and unpack circular references?} {
- r eval {local a = {x=nil,y=5}
- local b = {x=a}
- a['x'] = b
- local encoded = cmsgpack.pack(a)
- local h = ""
- -- cmsgpack encodes to a depth of 16, but can't encode
- -- references, so the encoded object has a deep copy recursive
- -- depth of 16.
- for i = 1, #encoded do
- h = h .. string.format("%02x",string.byte(encoded,i))
- end
- -- when unpacked, re.x.x != re because the unpack creates
- -- individual tables down to a depth of 16.
- -- (that's why the encoded output is so large)
- local re = cmsgpack.unpack(encoded)
- assert(re)
- assert(re.x)
- assert(re.x.x.y == re.y)
- assert(re.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.y == re.y)
- assert(re.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- maximum working depth:
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.y == re.y)
- -- now the last x would be b above and has no y
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
- -- so, the final x.x is at the depth limit and was assigned nil
- assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
- } 0
- } {82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0 1 1}
+ #test {EVAL - cmsgpack can pack and unpack circular references?} {
+ # r eval {local a = {x=nil,y=5}
Review Comment:
cool, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910659060
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
I had fix this test. The case is from here https://github.com/antirez/lua-cmsgpack/blob/master/test.lua#L393
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r910682121
##########
tests/tcl/tests/unit/scripting.tcl:
##########
@@ -263,9 +263,10 @@ start_server {tags {"scripting"}} {
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x)
-- so, the final x.x is at the depth limit and was assigned nil
assert(re.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x == nil)
- return {h, re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
+ assert(h == "82a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a17882a17881a178c0a17905a17905a17905a17905a17905a17905a17905a17905" or h == "82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0")
Review Comment:
> Yes， It behaves differently in different compilers
I still have deep doubts about this, because I think we should try to avoid semantics behaving differently under different compilers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1175836105
> As I commented in discussion, Redis did some changes to lua codebases, not only lua extensions. You can check [here](https://github.com/redis/redis/commits/unstable/deps/lua/src) to see these commits. I haven't investigated carefully what all these changes affect, and this may need some work.
>
> Other comments are inline.
[here](https://github.com/redis/redis/commits/unstable/deps/lua/src) The part of the lua code is modified for Redis 7.0, mainly the function function. I don't think this part needs to be ported to KVROCKS at this time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1179632060
> Maybe you need to add `*.hpp` to line 42 in lua.cmake to pass the "Ubuntu GCC without luajit" job.
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1190155223
@tisonkun Agree, I also think this is a big change and we can release it on the next release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1170887064
BTW, We can do the Luajit extension based on this repository. https://github.com/openresty/luajit2.
The repo add some new function https://github.com/openresty/luajit2#new-lua-apis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#issuecomment-1173031254
This is mem leak on my ubuntu
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r912483772
##########
src/server.cc:
##########
@@ -174,6 +174,7 @@ Status Server::Start() {
void Server::Stop() {
stop_ = true;
+ Lua::DestroyState(lua_);
Review Comment:
Can move this line to the server destructor and delete the server after the server threads join.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911537047
##########
src/scripting.h:
##########
@@ -23,9 +23,7 @@
#include <string>
#include <vector>
-#include <lua.h>
-#include <lauxlib.h>
-#include <lualib.h>
+#include <lua.hpp>
Review Comment:
http://luajit.org/extensions.html#exceptions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915025827
##########
src/server.h:
##########
@@ -30,7 +30,13 @@
#include <memory>
#include <unordered_map>
+#ifdef USE_LUAJIT
+#include <lua.hpp>
+#else
+#include <lauxlib.h>
#include <lua.h>
+#include <lualib.h>
+#endif
Review Comment:
With https://github.com/KvrocksLabs/lua/pull/4, we do not need `USE_LUAJIT` definition checking. Could you help to review & merge it? @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r911113057
##########
cmake/luajit.cmake:
##########
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+include_guard()
+
+include(FetchContent)
+
+FetchContent_Declare(luajit
+ GIT_REPOSITORY https://github.com/xiaobiaozhao/LuaJIT.git
+ GIT_TAG 803487f8b01c672495a2fcd29dcbed09e4fd6319
+)
+
+FetchContent_GetProperties(luajit)
+if(NOT lua_POPULATED)
+ FetchContent_Populate(luajit)
+
+ set(LUA_CFLAGS "-DLUA_ANSI -DENABLE_CJSON_GLOBAL -DREDIS_STATIC= -DLUA_USE_MKSTEMP")
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
+ set(LUA_CFLAGS "${LUA_CFLAGS} -isysroot ${CMAKE_OSX_SYSROOT}")
+ endif()
+
+ if (CMAKE_HOST_APPLE)
+ add_custom_target(make_luajit COMMAND make libluajit.a
+ "CFLAGS=${LUA_CFLAGS}"
+ MACOSX_DEPLOYMENT_TARGET=11.0
+ WORKING_DIRECTORY ${luajit_SOURCE_DIR}/src
+ BYPRODUCTS ${luajit_SOURCE_DIR}/src/libluajit.a
+ )
+ else()
+ add_custom_target(make_luajit COMMAND make libluajit.a
+ "CFLAGS=${LUA_CFLAGS}"
+ WORKING_DIRECTORY ${luajit_SOURCE_DIR}/src
+ BYPRODUCTS ${luajit_SOURCE_DIR}/src/libluajit.a
+ )
+ endif()
Review Comment:
done https://github.com/apache/incubator-kvrocks/pull/697/commits/33e874e0e4acc93dac2f7a9b4d5bdf9cf56fe37e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915890155
##########
src/server.h:
##########
@@ -30,7 +30,13 @@
#include <memory>
#include <unordered_map>
+#ifdef USE_LUAJIT
+#include <lua.hpp>
+#else
+#include <lauxlib.h>
#include <lua.h>
+#include <lualib.h>
+#endif
Review Comment:
sure, thanks @PragmaTwice
@xiaobiaozhao you can update the newest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r915890155
##########
src/server.h:
##########
@@ -30,7 +30,13 @@
#include <memory>
#include <unordered_map>
+#ifdef USE_LUAJIT
+#include <lua.hpp>
+#else
+#include <lauxlib.h>
#include <lua.h>
+#include <lualib.h>
+#endif
Review Comment:
sure, thanks @PragmaTwice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #697: ✨ feat: luajit replace lua
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #697:
URL: https://github.com/apache/incubator-kvrocks/pull/697#discussion_r916552541
##########
.github/workflows/kvrocks.yaml:
##########
@@ -101,6 +101,9 @@ jobs:
os: ubuntu-18.04
with_ninja: --ninja
without_jemalloc: -DDISABLE_JEMALLOC=ON
+ - name: Ubuntu GCC without luajit
+ os: ubuntu-18.04
+ without_luajit: -DUSE_LUAJIT=OFF
Review Comment:
I think you need to add `${{ matrix.without_luajit }}` to line 140, otherwise it is useless
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org