Skip to content

Commit

Permalink
Addresses reviewers' comments
Browse files Browse the repository at this point in the history 
* renames the prefix of scripting engine functions from `engine` to `scriptingEngine`
* fixes scripting_engine header file top macro name
* reorders source file list in cmake

Signed-off-by: Ricardo Dias <[email protected]>
  • Loading branch information
@rjd15372
rjd15372 committed Jan 3, 2025
1 parent 8f4e350 commit f4477c9
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 93 deletions.
4 changes: 2 additions & 2 deletions cmake/Modules/SourceFiles.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ set(VALKEY_SERVER_SRCS
${CMAKE_SOURCE_DIR}/src/script_lua.c
${CMAKE_SOURCE_DIR}/src/script.c
${CMAKE_SOURCE_DIR}/src/functions.c
${CMAKE_SOURCE_DIR}/src/scripting_engine.c
${CMAKE_SOURCE_DIR}/src/function_lua.c
${CMAKE_SOURCE_DIR}/src/commands.c
${CMAKE_SOURCE_DIR}/src/strl.c
${CMAKE_SOURCE_DIR}/src/connection.c
${CMAKE_SOURCE_DIR}/src/unix.c
${CMAKE_SOURCE_DIR}/src/server.c
${CMAKE_SOURCE_DIR}/src/logreqres.c
${CMAKE_SOURCE_DIR}/src/scripting_engine.c)
${CMAKE_SOURCE_DIR}/src/logreqres.c)

# valkey-cli
set(VALKEY_CLI_SRCS
Expand Down
2 changes: 1 addition & 1 deletion src/function_lua.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ int luaEngineInitEngine(void) {
.get_memory_info = luaEngineGetMemoryInfo,
};

return engineManagerRegisterEngine(LUA_ENGINE_NAME,
return scriptingEngineManagerRegister(LUA_ENGINE_NAME,
NULL,
lua_engine_ctx,
&lua_engine_methods);
Expand Down
40 changes: 20 additions & 20 deletions src/functions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ static size_t functionMallocSize(functionInfo *fi) {
return zmalloc_size(fi) +
sdsAllocSize(fi->name) +
(fi->desc ? sdsAllocSize(fi->desc) : 0) +
engineCallGetFunctionMemoryOverhead(fi->li->engine, fi->function);
scriptingEngineCallGetFunctionMemoryOverhead(fi->li->engine, fi->function);
}

static size_t libraryMallocSize(functionLibInfo *li) {
Expand All @@ -130,7 +130,7 @@ static void engineFunctionDispose(void *obj) {
sdsfree(fi->desc);
}

engineCallFreeFunction(fi->li->engine, fi->function);
scriptingEngineCallFreeFunction(fi->li->engine, fi->function);
zfree(fi);
}

Expand Down Expand Up @@ -206,7 +206,7 @@ functionsLibCtx *functionsLibCtxGetCurrent(void) {
static int initializeFunctionsLibEngineStats(scriptingEngine *engine, void *context) {
functionsLibCtx *lib_ctx = (functionsLibCtx *)context;
functionsLibEngineStats *stats = zcalloc(sizeof(*stats));
dictAdd(lib_ctx->engines_stats, engineGetName(engine), stats);
dictAdd(lib_ctx->engines_stats, scriptingEngineGetName(engine), stats);
return 1;
}

Expand All @@ -216,7 +216,7 @@ functionsLibCtx *functionsLibCtxCreate(void) {
ret->libraries = dictCreate(&librariesDictType);
ret->functions = dictCreate(&functionDictType);
ret->engines_stats = dictCreate(&engineStatsDictType);
engineManagerForEachEngine(initializeFunctionsLibEngineStats, ret);
scriptingEngineManagerForEachEngine(initializeFunctionsLibEngineStats, ret);
ret->cache_memory = 0;
return ret;
}
Expand Down Expand Up @@ -303,7 +303,7 @@ static void libraryUnlink(functionsLibCtx *lib_ctx, functionLibInfo *li) {
lib_ctx->cache_memory -= libraryMallocSize(li);

/* update stats */
functionsLibEngineStats *stats = dictFetchValue(lib_ctx->engines_stats, engineGetName(li->engine));
functionsLibEngineStats *stats = dictFetchValue(lib_ctx->engines_stats, scriptingEngineGetName(li->engine));
serverAssert(stats);
stats->n_lib--;
stats->n_functions -= dictSize(li->functions);
Expand All @@ -323,7 +323,7 @@ static void libraryLink(functionsLibCtx *lib_ctx, functionLibInfo *li) {
lib_ctx->cache_memory += libraryMallocSize(li);

/* update stats */
functionsLibEngineStats *stats = dictFetchValue(lib_ctx->engines_stats, engineGetName(li->engine));
functionsLibEngineStats *stats = dictFetchValue(lib_ctx->engines_stats, scriptingEngineGetName(li->engine));
serverAssert(stats);
stats->n_lib++;
stats->n_functions += dictSize(li->functions);
Expand Down Expand Up @@ -412,10 +412,10 @@ libraryJoin(functionsLibCtx *functions_lib_ctx_dst, functionsLibCtx *functions_l

static int replyEngineStats(scriptingEngine *engine, void *context) {
client *c = (client *)context;
addReplyBulkCString(c, engineGetName(engine));
addReplyBulkCString(c, scriptingEngineGetName(engine));
addReplyMapLen(c, 2);
functionsLibEngineStats *e_stats =
dictFetchValue(curr_functions_lib_ctx->engines_stats, engineGetName(engine));
dictFetchValue(curr_functions_lib_ctx->engines_stats, scriptingEngineGetName(engine));
addReplyBulkCString(c, "libraries_count");
addReplyLongLong(c, e_stats ? e_stats->n_lib : 0);
addReplyBulkCString(c, "functions_count");
Expand Down Expand Up @@ -465,8 +465,8 @@ void functionStatsCommand(client *c) {
}

addReplyBulkCString(c, "engines");
addReplyMapLen(c, engineManagerGetNumEngines());
engineManagerForEachEngine(replyEngineStats, c);
addReplyMapLen(c, scriptingEngineManagerGetNumEngines());
scriptingEngineManagerForEachEngine(replyEngineStats, c);
}

static void functionListReplyFlags(client *c, functionInfo *fi) {
Expand Down Expand Up @@ -542,7 +542,7 @@ void functionListCommand(client *c) {
addReplyBulkCString(c, "library_name");
addReplyBulkCBuffer(c, li->name, sdslen(li->name));
addReplyBulkCString(c, "engine");
sds engine_name = engineGetName(li->engine);
sds engine_name = scriptingEngineGetName(li->engine);
addReplyBulkCBuffer(c, engine_name, sdslen(engine_name));

addReplyBulkCString(c, "functions");
Expand Down Expand Up @@ -640,9 +640,9 @@ static void fcallCommandGeneric(client *c, int ro) {
}

scriptRunCtx run_ctx;
if (scriptPrepareForRun(&run_ctx, engineGetClient(engine), c, fi->name, fi->f_flags, ro) != C_OK) return;
if (scriptPrepareForRun(&run_ctx, scriptingEngineGetClient(engine), c, fi->name, fi->f_flags, ro) != C_OK) return;

engineCallFunction(engine,
scriptingEngineCallFunction(engine,
&run_ctx,
run_ctx.original_client,
fi->function,
Expand Down Expand Up @@ -960,7 +960,7 @@ static void freeCompiledFunctions(scriptingEngine *engine,
decrRefCount(func->desc);
}
if (i >= free_function_from_idx) {
engineCallFreeFunction(engine, func->function);
scriptingEngineCallFreeFunction(engine, func->function);
}
zfree(func);
}
Expand All @@ -987,7 +987,7 @@ sds functionsCreateWithLibraryCtx(sds code, int replace, sds *err, functionsLibC
goto error;
}

scriptingEngine *engine = engineManagerFind(md.engine);
scriptingEngine *engine = scriptingEngineManagerFind(md.engine);
if (!engine) {
*err = sdscatfmt(sdsempty(), "Engine '%S' not found", md.engine);
goto error;
Expand All @@ -1010,7 +1010,7 @@ sds functionsCreateWithLibraryCtx(sds code, int replace, sds *err, functionsLibC
size_t num_compiled_functions = 0;
robj *compile_error = NULL;
compiledFunction **compiled_functions =
engineCallCreateFunctionsLibrary(engine,
scriptingEngineCallCreateFunctionsLibrary(engine,
md.code,
timeout,
&num_compiled_functions,
Expand Down Expand Up @@ -1126,25 +1126,25 @@ void functionLoadCommand(client *c) {

static int getEngineUsedMemory(scriptingEngine *engine, void *context) {
size_t *engines_memory = (size_t *)context;
engineMemoryInfo mem_info = engineCallGetMemoryInfo(engine);
engineMemoryInfo mem_info = scriptingEngineCallGetMemoryInfo(engine);
*engines_memory += mem_info.used_memory;
return 1;
}

/* Return memory usage of all the engines combine */
unsigned long functionsMemory(void) {
size_t engines_memory = 0;
engineManagerForEachEngine(getEngineUsedMemory, &engines_memory);
scriptingEngineManagerForEachEngine(getEngineUsedMemory, &engines_memory);
return engines_memory;
}

/* Return memory overhead of all the engines combine */
unsigned long functionsMemoryOverhead(void) {
size_t memory_overhead = engineManagerGetMemoryUsage();
size_t memory_overhead = scriptingEngineManagerGetMemoryUsage();
memory_overhead += dictMemUsage(curr_functions_lib_ctx->functions);
memory_overhead += sizeof(functionsLibCtx);
memory_overhead += curr_functions_lib_ctx->cache_memory;
memory_overhead += engineManagerGetCacheMemory();
memory_overhead += scriptingEngineManagerGetCacheMemory();

return memory_overhead;
}
Expand Down
4 changes: 2 additions & 2 deletions src/module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13125,7 +13125,7 @@ int VM_RegisterScriptingEngine(ValkeyModuleCtx *module_ctx,
return VALKEYMODULE_ERR;
}

if (engineManagerRegisterEngine(engine_name,
if (scriptingEngineManagerRegister(engine_name,
module_ctx->module,
engine_ctx,
engine_methods) != C_OK) {
Expand All @@ -13144,7 +13144,7 @@ int VM_RegisterScriptingEngine(ValkeyModuleCtx *module_ctx,
*/
int VM_UnregisterScriptingEngine(ValkeyModuleCtx *ctx, const char *engine_name) {
UNUSED(ctx);
if (engineManagerUnregisterEngine(engine_name) != C_OK) {
if (scriptingEngineManagerUnregister(engine_name) != C_OK) {
return VALKEYMODULE_ERR;
}
return VALKEYMODULE_OK;
Expand Down
67 changes: 34 additions & 33 deletions src/scripting_engine.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,20 @@ dictType engineDictType = {
*
* Returns C_ERR if some error occurs during the initialization.
*/
int engineManagerInit(void) {
int scriptingEngineManagerInit(void) {
engineMgr.engines = dictCreate(&engineDictType);
return C_OK;
}

size_t engineManagerGetCacheMemory(void) {
size_t scriptingEngineManagerGetCacheMemory(void) {
return engineMgr.engine_cache_memory;
}

size_t engineManagerGetNumEngines(void) {
size_t scriptingEngineManagerGetNumEngines(void) {
return dictSize(engineMgr.engines);
}

size_t engineManagerGetMemoryUsage(void) {
size_t scriptingEngineManagerGetMemoryUsage(void) {
return dictMemUsage(engineMgr.engines) + sizeof(engineMgr);
}

Expand All @@ -79,10 +79,10 @@ size_t engineManagerGetMemoryUsage(void) {
*
* Returns C_ERR in case of an error during registration.
*/
int engineManagerRegisterEngine(const char *engine_name,
ValkeyModule *engine_module,
engineCtx *engine_ctx,
engineMethods *engine_methods) {
int scriptingEngineManagerRegister(const char *engine_name,
ValkeyModule *engine_module,
engineCtx *engine_ctx,
engineMethods *engine_methods) {
sds engine_name_sds = sdsnew(engine_name);

if (dictFetchValue(engineMgr.engines, engine_name_sds)) {
Expand Down Expand Up @@ -119,7 +119,7 @@ int engineManagerRegisterEngine(const char *engine_name,

dictAdd(engineMgr.engines, engine_name_sds, e);

engineMemoryInfo mem_info = engineCallGetMemoryInfo(e);
engineMemoryInfo mem_info = scriptingEngineCallGetMemoryInfo(e);
engineMgr.engine_cache_memory += zmalloc_size(e) +
sdsAllocSize(e->name) +
zmalloc_size(ei) +
Expand All @@ -132,7 +132,7 @@ int engineManagerRegisterEngine(const char *engine_name,
*
* - `engine_name`: name of the engine to remove
*/
int engineManagerUnregisterEngine(const char *engine_name) {
int scriptingEngineManagerUnregister(const char *engine_name) {
dictEntry *entry = dictUnlink(engineMgr.engines, engine_name);
if (entry == NULL) {
serverLog(LL_WARNING, "There's no engine registered with name %s", engine_name);
Expand Down Expand Up @@ -161,23 +161,23 @@ int engineManagerUnregisterEngine(const char *engine_name) {
* Lookups the engine with `engine_name` in the engine manager and returns it if
* it exists. Otherwise returns `NULL`.
*/
scriptingEngine *engineManagerFind(sds engine_name) {
scriptingEngine *scriptingEngineManagerFind(sds engine_name) {
dictEntry *entry = dictFind(engineMgr.engines, engine_name);
if (entry) {
return dictGetVal(entry);
}
return NULL;
}

sds engineGetName(scriptingEngine *engine) {
sds scriptingEngineGetName(scriptingEngine *engine) {
return engine->name;
}

client *engineGetClient(scriptingEngine *engine) {
client *scriptingEngineGetClient(scriptingEngine *engine) {
return engine->c;
}

ValkeyModule *engineGetModule(scriptingEngine *engine) {
ValkeyModule *scriptingEngineGetModule(scriptingEngine *engine) {
return engine->module;
}

Expand All @@ -187,7 +187,8 @@ ValkeyModule *engineGetModule(scriptingEngine *engine) {
*
* The `context` pointer is also passed in each callback call.
*/
void engineManagerForEachEngine(engineIterCallback callback, void *context) {
void scriptingEngineManagerForEachEngine(engineIterCallback callback,
void *context) {
dictIterator *iter = dictGetIterator(engineMgr.engines);
dictEntry *entry = NULL;
while ((entry = dictNext(iter))) {
Expand All @@ -213,11 +214,11 @@ static void engineTeardownModuleCtx(scriptingEngine *e) {
}
}

compiledFunction **engineCallCreateFunctionsLibrary(scriptingEngine *engine,
const char *code,
size_t timeout,
size_t *out_num_compiled_functions,
robj **err) {
compiledFunction **scriptingEngineCallCreateFunctionsLibrary(scriptingEngine *engine,
const char *code,
size_t timeout,
size_t *out_num_compiled_functions,
robj **err) {
engineSetupModuleCtx(engine, NULL);

compiledFunction **functions = engine->impl->methods.create_functions_library(
Expand All @@ -233,14 +234,14 @@ compiledFunction **engineCallCreateFunctionsLibrary(scriptingEngine *engine,
return functions;
}

void engineCallFunction(scriptingEngine *engine,
functionCtx *func_ctx,
client *caller,
void *compiled_function,
robj **keys,
size_t nkeys,
robj **args,
size_t nargs) {
void scriptingEngineCallFunction(scriptingEngine *engine,
functionCtx *func_ctx,
client *caller,
void *compiled_function,
robj **keys,
size_t nkeys,
robj **args,
size_t nargs) {
engineSetupModuleCtx(engine, caller);

engine->impl->methods.call_function(
Expand All @@ -256,25 +257,25 @@ void engineCallFunction(scriptingEngine *engine,
engineTeardownModuleCtx(engine);
}

void engineCallFreeFunction(scriptingEngine *engine,
void *compiled_func) {
void scriptingEngineCallFreeFunction(scriptingEngine *engine,
void *compiled_func) {
engineSetupModuleCtx(engine, NULL);
engine->impl->methods.free_function(engine->module_ctx,
engine->impl->ctx,
compiled_func);
engineTeardownModuleCtx(engine);
}

size_t engineCallGetFunctionMemoryOverhead(scriptingEngine *engine,
void *compiled_function) {
size_t scriptingEngineCallGetFunctionMemoryOverhead(scriptingEngine *engine,
void *compiled_function) {
engineSetupModuleCtx(engine, NULL);
size_t mem = engine->impl->methods.get_function_memory_overhead(
engine->module_ctx, compiled_function);
engineTeardownModuleCtx(engine);
return mem;
}

engineMemoryInfo engineCallGetMemoryInfo(scriptingEngine *engine) {
engineMemoryInfo scriptingEngineCallGetMemoryInfo(scriptingEngine *engine) {
engineSetupModuleCtx(engine, NULL);
engineMemoryInfo mem_info = engine->impl->methods.get_memory_info(
engine->module_ctx, engine->impl->ctx);
Expand Down
Loading

0 comments on commit f4477c9

Please sign in to comment.