✨ feat: luajit replace lua #697

xiaobiaozhao · 2022-06-29T14:34:24Z

Luajit replace lua
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).

PragmaTwice · 2022-06-29T15:33:36Z

Thanks for your hard work.

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 needs to be solved.

tests/tcl/tests/unit/scripting.tcl

PragmaTwice · 2022-06-30T04:32:43Z

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


Why there are two different values of h?

I had fix this test. The case is from here https://github.com/antirez/lua-cmsgpack/blob/master/test.lua#L393

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

Yes， It behaves differently in different compilers

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.

PragmaTwice

As I commented in discussion, Redis did some changes to lua codebases, not only lua extensions. You can check here to see these commits. I haven't investigated carefully what all these changes affect, and this may need some work.

Other comments are inline.

CMakeLists.txt

cmake/luajit.cmake

src/scripting.h

xiaobiaozhao · 2022-06-30T07:51:21Z

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

call luaDestroyState in ~server

PragmaTwice · 2022-07-02T07:12:08Z

In order to use sanitizers, you not only need to add compile options, but also link options. (which may be why CI is currently failing)
Also, don't forget to remove these sanitizer options when the bug is fixed : )

xiaobiaozhao · 2022-07-03T07:46:17Z

This is mem leak on my ubuntu

git-hulk · 2022-07-03T13:00:39Z

Many thanks for @xiaobiaozhao hard try

mv Lua::DestroyState to ~Server

xiaobiaozhao · 2022-07-06T06:35:25Z

As I commented in discussion, Redis did some changes to lua codebases, not only lua extensions. You can check here 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 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

PragmaTwice · 2022-07-06T06:58:38Z

here 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: #697 (comment) ?
I think it is not a hard job to support both since all these APIs are same.

If users encounter incompatibility between luajit and lua that makes the previous code unable to execute properly, or other problems with luajit, they can choose to switch to original lua to avoid it.

git-hulk · 2022-07-06T09:58:16Z

@tisonkun and @ShooterIT can take a look if you have time. For #697 (comment), I also prefer providing an option to let user fallback if they have any problem.

General implementation is good to me.

CMakeLists.txt

src/server.h

PragmaTwice · 2022-07-09T13:41:17Z

Maybe you need to add *.hpp to line 42 in lua.cmake to pass the "Ubuntu GCC without luajit" job.

xiaobiaozhao · 2022-07-10T01:07:51Z

Maybe you need to add *.hpp to line 42 in lua.cmake to pass the "Ubuntu GCC without luajit" job.

done

PragmaTwice · 2022-07-13T16:01:30Z

There are some memory leaks reported by ASan which are steadily reproduced and need some investigation.

The rest is good for me. 👍

git-hulk · 2022-07-13T16:24:14Z

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.

PragmaTwice · 2022-07-14T06:59:31Z

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

git-hulk · 2022-07-19T08:59:36Z

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 fixed those issues in #714 and it looks good now, I think this PR would be also became green after #714.

git-hulk · 2022-07-20T10:38:06Z

@tisonkun @PragmaTwice @ShooterIT I think we can merge this PR if no objections.

tisonkun

LGTM. Thank you very much for this fruitful contribution @xiaobiaozhao!

@git-hulk one thing to point out is that we may not include this change in 2.1.0 release and at earliest in 2.2.0.

tisonkun

LGTM. Thank you very much for this fruitful contribution @xiaobiaozhao!

@git-hulk one thing to point out is that we may not include this change in 2.1.0 release and at earliest in 2.2.0.

PragmaTwice

LGTM, thanks for such a great work.

git-hulk · 2022-07-20T11:25:54Z

@tisonkun Agree, I also think this is a big change and we can release it on the next release.

git-hulk · 2022-07-20T11:38:31Z

Thanks all, merging...

git-hulk · 2022-07-20T11:43:34Z

Thanks for @xiaobiaozhao great contribution again.

git-hulk reviewed Jun 30, 2022

View reviewed changes

tests/tcl/tests/unit/scripting.tcl Outdated Show resolved Hide resolved

✨ feat: luajit replace lua

7535a51

xiaobiaozhao force-pushed the luajit branch from 37dc490 to 7535a51 Compare June 30, 2022 03:43

PragmaTwice reviewed Jun 30, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

cmake/luajit.cmake Outdated Show resolved Hide resolved

cmake/luajit.cmake Outdated Show resolved Hide resolved

cmake/luajit.cmake Outdated Show resolved Hide resolved

src/scripting.h Outdated Show resolved Hide resolved

xiaobiaozhao added 2 commits June 30, 2022 21:47

✨ feat: update luajit.cmake

33e874e

✨ feat: mod luajit src

c4a6697

tisonkun mentioned this pull request Jul 1, 2022

Memory leak reports in tcl test cases #702

Closed

2 tasks

git-hulk requested review from ShooterIT and tisonkun July 1, 2022 06:37

xiaobiaozhao added 2 commits July 1, 2022 22:07

🐞 fix: fix mem leak

707b177

call luaDestroyState in ~server

🐞 fix: more lua-cflag

c20070d

add asan

27a507c

🐞 fix: fix luajit mem leak

d2654cd

xiaobiaozhao and others added 2 commits July 3, 2022 21:34

🦄 refactor: optimize

41a2bcb

mv Lua::DestroyState to ~Server

Merge branch 'unstable' into luajit

cb3de60

xiaobiaozhao added 2 commits July 6, 2022 23:17

✨ feat: add cmake option

e3d612d

🐞 fix:luajit cmake

5df804f

PragmaTwice reviewed Jul 6, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

PragmaTwice reviewed Jul 6, 2022

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

PragmaTwice and others added 2 commits July 7, 2022 22:40

Merge branch 'unstable' into luajit

1bbaa2e

fix typo in CMakeLists.txt

870ab9d

🐞 fix: add *.hpp to lua.cmake

ccfbd4f

git-hulk previously approved these changes Jul 12, 2022

View reviewed changes

git-hulk and others added 2 commits July 12, 2022 10:11

Merge branch 'unstable' into luajit

4e542c6

Merge branch 'unstable' into luajit

61eddc5

tisonkun dismissed git-hulk’s stale review via 61eddc5 July 12, 2022 14:45

tisonkun previously approved these changes Jul 13, 2022

View reviewed changes

git-hulk previously approved these changes Jul 13, 2022

View reviewed changes

Merge branch 'unstable' into luajit

eedfada

Merge branch 'unstable' into luajit

ba54b75

PragmaTwice dismissed stale reviews from git-hulk and tisonkun via ba54b75 July 20, 2022 08:38

git-hulk requested review from PragmaTwice, tisonkun and git-hulk July 20, 2022 08:45

git-hulk approved these changes Jul 20, 2022

View reviewed changes

tisonkun approved these changes Jul 20, 2022

View reviewed changes

PragmaTwice approved these changes Jul 20, 2022

View reviewed changes

ShooterIT approved these changes Jul 20, 2022

View reviewed changes

git-hulk merged commit bacb45a into apache:unstable Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feat: luajit replace lua #697

✨ feat: luajit replace lua #697

xiaobiaozhao commented Jun 29, 2022

PragmaTwice commented Jun 29, 2022 •

edited

Loading

PragmaTwice Jun 30, 2022 •

edited

Loading

xiaobiaozhao Jun 30, 2022

PragmaTwice Jun 30, 2022

xiaobiaozhao Jun 30, 2022

PragmaTwice Jun 30, 2022

PragmaTwice left a comment

xiaobiaozhao commented Jun 30, 2022

PragmaTwice commented Jul 2, 2022 •

edited

Loading

xiaobiaozhao commented Jul 3, 2022

git-hulk commented Jul 3, 2022

xiaobiaozhao commented Jul 6, 2022 •

edited

Loading

PragmaTwice commented Jul 6, 2022 •

edited

Loading

git-hulk commented Jul 6, 2022 •

edited

Loading

PragmaTwice commented Jul 9, 2022

xiaobiaozhao commented Jul 10, 2022

PragmaTwice commented Jul 13, 2022

git-hulk commented Jul 13, 2022

PragmaTwice commented Jul 14, 2022 •

edited

Loading

git-hulk commented Jul 19, 2022 •

edited

Loading

git-hulk commented Jul 20, 2022

tisonkun left a comment

tisonkun left a comment

PragmaTwice left a comment •

edited

Loading

git-hulk commented Jul 20, 2022

git-hulk commented Jul 20, 2022

git-hulk commented Jul 20, 2022

✨ feat: luajit replace lua #697

✨ feat: luajit replace lua #697

Conversation

xiaobiaozhao commented Jun 29, 2022

PragmaTwice commented Jun 29, 2022 • edited Loading

PragmaTwice Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

xiaobiaozhao Jun 30, 2022

Choose a reason for hiding this comment

PragmaTwice Jun 30, 2022

Choose a reason for hiding this comment

xiaobiaozhao Jun 30, 2022

Choose a reason for hiding this comment

PragmaTwice Jun 30, 2022

Choose a reason for hiding this comment

PragmaTwice left a comment

Choose a reason for hiding this comment

xiaobiaozhao commented Jun 30, 2022

PragmaTwice commented Jul 2, 2022 • edited Loading

xiaobiaozhao commented Jul 3, 2022

git-hulk commented Jul 3, 2022

xiaobiaozhao commented Jul 6, 2022 • edited Loading

PragmaTwice commented Jul 6, 2022 • edited Loading

git-hulk commented Jul 6, 2022 • edited Loading

PragmaTwice commented Jul 9, 2022

xiaobiaozhao commented Jul 10, 2022

PragmaTwice commented Jul 13, 2022

git-hulk commented Jul 13, 2022

PragmaTwice commented Jul 14, 2022 • edited Loading

git-hulk commented Jul 19, 2022 • edited Loading

git-hulk commented Jul 20, 2022

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

PragmaTwice left a comment • edited Loading

Choose a reason for hiding this comment

git-hulk commented Jul 20, 2022

git-hulk commented Jul 20, 2022

git-hulk commented Jul 20, 2022

PragmaTwice commented Jun 29, 2022 •

edited

Loading

PragmaTwice Jun 30, 2022 •

edited

Loading

PragmaTwice commented Jul 2, 2022 •

edited

Loading

xiaobiaozhao commented Jul 6, 2022 •

edited

Loading

PragmaTwice commented Jul 6, 2022 •

edited

Loading

git-hulk commented Jul 6, 2022 •

edited

Loading

PragmaTwice commented Jul 14, 2022 •

edited

Loading

git-hulk commented Jul 19, 2022 •

edited

Loading

PragmaTwice left a comment •

edited

Loading