- Notifications
You must be signed in to change notification settings - Fork386
perf: update 1mops to support memcs#11293
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
80225ef tocae7202Comparecoveralls commentedMar 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
8a2f2d8 to1e03950CompareNow it takes a new `columns' parameter describing the amount of fieldsin replaced tuples (the default is 1 so the behavior is not changed).NO_DOC=pref testNO_TEST=perf testNO_CHANGELOG=perf test
1e03950 to82658f5CompareThe new fiber creates raw tuples without need to convert lua objectsto MessagePack. The option increases the test accuracy, so let's makeit use the new fiber by default (if the module is available).NO_DOC=perf testNO_TEST=perf testNO_CHANGELOG=perf test
82658f5 tobd11faeCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Magomed, thanks for the patches! Please see my comments.
| @@ -10,6 +10,7 @@ local fiber = require('fiber') | |||
| local math = require('math') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typo in commit " perf: update the 1mops_write test to support memcs": s/pref/perf/
| @@ -247,17 +263,28 @@ if (res ~= true) then | |||
| json.encode(index_config) .. ' :' .. json.encode(err)) | |||
| end | |||
| -- Preallocate the test tuple | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Previous comments started from lowercase letter
| } | ||
| static int | ||
| fiber_lua_func(struct lua_State *L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems function has nothing common with fibers, I would rename it.
| local MODULEPATH = fio.pathjoin(BUILDDIR, 'perf', 'lua', | ||
| '?.' .. tarantool.build.mod_format) | ||
| package.cpath = MODULEPATH .. ';' .. package.cpath | ||
| local module_is_available, module = pcall(require, '1mops_write_module') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
s/module_is_available/1mops_write_module/
s/module/1mops_write_module/
and probably it is better to rename1mops_write_module to something that will reflect specific of this module. may becolumn_write_module?
| @@ -0,0 +1,173 @@ | |||
| #include <lua.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm a bit confused how many low-level memcs modules we already have:
perf/lua/1mops_write_module.cperf/lua/column_insert_module.cperf/lua/column_scan_module.cDo you ever plan to introduce a Lua API to memcs?
| } | ||
| LUA_API int | ||
| luaopen_1mops_write_module(struct lua_State *L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know how to ensure that1mops_write_module.c uses memcs API in the same way as TCS. Should we involve someone from TCS team to review?
| } | ||
| static bool | ||
| create_base_tuples(uint32_t num_columns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm confused that we test engine by a limit number of datatypes. Should we parametrize a test (or probably add another one) that will test memcs by different datatypes (standard msgpack types and our extensions1).
Footnotes
| @@ -10,6 +10,7 @@ local fiber = require('fiber') | |||
| local math = require('math') | |||
| local json = require('json') | |||
| local fio = require('fio') | |||
| local tarantool = require('tarantool') | |||
| -- XXX: This benchmark should be able to work standalone without | |||
| -- any provided libraries or set LUA_PATH. Add stubs for that | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typo in commit "perf: update the 1mops_write test to support memcs": s/pref/perf/
Now it takes a new `columns' parameter describing the amount of fields in replaced tuples (the default is 1 so the behavior is not changed).
NO_DOC=pref test
NO_TEST=perf test
NO_CHANGELOG=perf test