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

✨ feat: luajit replace lua #697

Merged
merged 22 commits into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/kvrocks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@PragmaTwice PragmaTwice Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add ${{ matrix.without_luajit }} to line 140, otherwise it is useless

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done d4a6ff9


runs-on: ${{ matrix.os }}
steps:
Expand All @@ -117,7 +120,7 @@ jobs:
run: |
brew install cmake gcc autoconf automake libtool
echo "NPROC=$(sysctl -n hw.ncpu)" >> $GITHUB_ENV

- name: Setup Linux
if: ${{ startsWith(matrix.os, 'ubuntu') }}
run: |
Expand All @@ -137,7 +140,7 @@ jobs:
uses: actions/checkout@v3

- name: Build Kvrocks
run: ./build.sh build -j$NPROC --unittest ${{ matrix.clang }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} ${{ matrix.without_jemalloc }}
run: ./build.sh build -j$NPROC --unittest ${{ matrix.clang }} ${{ matrix.with_ninja }} ${{ matrix.with_sanitizer }} ${{ matrix.without_jemalloc }} ${{ matrix.without_luajit }}

- name: Run Unit Test
run: ./build/unittest
Expand Down
16 changes: 14 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 "use luaJIT instead of lua" ON)

set(DEPS_FETCH_PROXY "" CACHE STRING
set(DEPS_FETCH_PROXY "" CACHE STRING
"a template URL to proxy the traffic for fetching dependencies, e.g. with DEPS_FETCH_PROXY = https://some-proxy/,
https://example/some-dep.zip -> https://some-proxy/https://example/some-dep.zip")

Expand Down Expand Up @@ -59,16 +60,27 @@ include(cmake/snappy.cmake)
include(cmake/lz4.cmake)
include(cmake/rocksdb.cmake)
include(cmake/libevent.cmake)

if (USE_LUAJIT)
include(cmake/luajit.cmake)
PragmaTwice marked this conversation as resolved.
Show resolved Hide resolved
else()
include(cmake/lua.cmake)
endif()

find_package(Threads REQUIRED)

list(APPEND EXTERNAL_LIBS PRIVATE glog)
list(APPEND EXTERNAL_LIBS PRIVATE snappy)
list(APPEND EXTERNAL_LIBS PRIVATE rocksdb_with_headers)
list(APPEND EXTERNAL_LIBS PRIVATE event_with_headers)
list(APPEND EXTERNAL_LIBS PRIVATE lua)
list(APPEND EXTERNAL_LIBS PRIVATE lz4)
if (USE_LUAJIT)
list(APPEND EXTERNAL_LIBS PRIVATE luajit)
else()
list(APPEND EXTERNAL_LIBS PRIVATE lua)
endif()


list(APPEND EXTERNAL_LIBS PRIVATE Threads::Threads)

# Add git sha to version.h
Expand Down
8 changes: 4 additions & 4 deletions cmake/lua.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ include_guard()
include(cmake/utils.cmake)

FetchContent_DeclareGitHubWithMirror(lua
KvrocksLabs/lua c8e4bbfa25f7202f3b778ccb88e54ab84a1861fb
MD5=79950ff054ae76e5c9f9c57b38484229
KvrocksLabs/lua f458c3d797db31155fa0c156d5301716df48cb8c
MD5=c7c4deb9f750d8f2bef0044a701df85c
)

FetchContent_GetProperties(lua)
Expand All @@ -38,8 +38,8 @@ if(NOT lua_POPULATED)
WORKING_DIRECTORY ${lua_SOURCE_DIR}/src
BYPRODUCTS ${lua_SOURCE_DIR}/src/liblua.a
)
file(GLOB LUA_PUBLIC_HEADERS "${lua_SOURCE_DIR}/src/*.h")

file(GLOB LUA_PUBLIC_HEADERS "${lua_SOURCE_DIR}/src/*.h" "${lua_SOURCE_DIR}/src/*.hpp")
file(COPY ${LUA_PUBLIC_HEADERS} DESTINATION ${lua_BINARY_DIR}/include)
endif()

Expand Down
55 changes: 55 additions & 0 deletions cmake/luajit.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# 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(cmake/utils.cmake)

FetchContent_DeclareGitHubWithMirror(luajit
KvrocksLabs/LuaJIT 803487f8b01c672495a2fcd29dcbed09e4fd6319
MD5=cd08841342cd933fb7e3d6d4253fbeec
)

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()

set(MACOSX_TARGET "")
if (CMAKE_HOST_APPLE)
set(MACOSX_TARGET "MACOSX_DEPLOYMENT_TARGET=11.0")
endif()

add_custom_target(make_luajit COMMAND make libluajit.a
"CFLAGS=${LUA_CFLAGS}"
${MACOSX_TARGET}
WORKING_DIRECTORY ${luajit_SOURCE_DIR}/src
BYPRODUCTS ${luajit_SOURCE_DIR}/src/libluajit.a
)

file(GLOB LUA_PUBLIC_HEADERS "${luajit_SOURCE_DIR}/src/*.hpp" "${luajit_SOURCE_DIR}/src/*.h")
file(COPY ${LUA_PUBLIC_HEADERS} DESTINATION ${luajit_BINARY_DIR}/include)
endif()

add_library(luajit INTERFACE)
target_include_directories(luajit INTERFACE ${luajit_BINARY_DIR}/include)
target_link_libraries(luajit INTERFACE ${luajit_SOURCE_DIR}/src/libluajit.a dl)
add_dependencies(luajit make_luajit)
2 changes: 2 additions & 0 deletions src/main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,8 @@ int main(int argc, char* argv[]) {
}
srv->Join();

delete srv;

removePidFile(config.pidfile);
google::ShutdownGoogleLogging();
libevent_global_shutdown();
Expand Down
1 change: 1 addition & 0 deletions src/scripting.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ namespace Lua {
}

void DestroyState(lua_State *lua) {
lua_gc(lua, LUA_GCCOLLECT, 0);
lua_close(lua);
}

Expand Down
6 changes: 1 addition & 5 deletions src/scripting.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,10 @@
#include <string>
#include <vector>

#include <lua.h>
#include <lauxlib.h>
#include <lualib.h>

#include "lua.hpp"
#include "status.h"
#include "redis_connection.h"


namespace Lua {

lua_State* CreateState();
Expand Down
2 changes: 2 additions & 0 deletions src/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ Server::~Server() {
for (const auto &iter : conn_ctxs_) {
delete iter.first;
}

Lua::DestroyState(lua_);
}

// Kvrocks threads list:
Expand Down
3 changes: 1 addition & 2 deletions src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
#include <memory>
#include <unordered_map>

#include <lua.h>

#include "lua.hpp"
#include "stats.h"
#include "storage.h"
#include "task_runner.h"
Expand Down
5 changes: 3 additions & 2 deletions tests/tcl/tests/unit/scripting.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

@PragmaTwice PragmaTwice Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there are two different values of h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it. It is due to the element order of a table is unstable. This reminds me of Map in golang.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, It behaves differently in different compilers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return {re.x.x.x.x.x.x.x.x.y == re.y, re.y == 5}
} 0
} {82a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a17882a17905a17881a178c0 1 1}
} {1 1}
test {EVAL - Numerical sanity check from bitop} {
r eval {assert(0x7fffffff == 2147483647, "broken hex literals");
assert(0xffffffff == -1 or 0xffffffff == 2^32-1,
Expand Down