Exec plan cache #1117

DvirDukhan · 2020-06-08T12:59:44Z

No description provided.

jeffreylovitz · 2020-06-11T13:59:22Z

src/ast/ast.c

-	if(!root) return false;
+bool AST_ReadOnly(const cypher_astnode_t *root) {
+	// Check for empty query
+	if(root == NULL) return true;


Condition inverted? Does this make a difference?

This is the condition

RedisGraph/src/ast/ast.c

Line 149 in 32641dc

if(root == NULL) return true;

What's going on here, why was the condition swapped?

jeffreylovitz · 2020-06-11T14:04:29Z

src/ast/ast_validations.c

+	ast->params_parse_result = NULL;
+	ast->parse_result = NULL;
+	ast->ref_count = 1;


Reorder lines by length.

jeffreylovitz · 2020-06-11T14:06:27Z

src/ast/ast_shared.c

@@ -114,19 +114,19 @@ EntityUpdateEvalCtx EntityUpdateEvalCtx_Clone(EntityUpdateEvalCtx ctx) {
 }

 NodeCreateCtx NodeCreateCtx_Clone(NodeCreateCtx ctx) {
-	NodeCreateCtx clone;
+	NodeCreateCtx clone = {0};


This can optionally be dropped in favor of an explicit NULL-set of properties. I favor this approach so that we don't let later struct member additions get mistaken for initialized values.

jeffreylovitz · 2020-06-11T14:06:45Z

src/ast/ast_shared.c

 	return clone;
 }

 EdgeCreateCtx EdgeCreateCtx_Clone(EdgeCreateCtx ctx) {
-	EdgeCreateCtx clone;
+	EdgeCreateCtx clone = {0};


Same comment as at 117.

jeffreylovitz · 2020-06-11T14:07:38Z

src/commands/cmd_explain.c

@@ -6,6 +6,7 @@

 #include "cmd_explain.h"
 #include "cmd_context.h"
+#include "execution_ctx.h"


Nit - reorder lines by length.

jeffreylovitz · 2020-06-11T16:06:17Z

src/ast/ast.h

+// Sets a parameter parsing result in the ast.
+void AST_SetParamsParseResult(AST *ast, cypher_parse_result_t *params_parse_result);
+
+// Returns a shallow clone (ref counter increase) of the original AST.


Rephrase comment to more accurately represent what the function does.

jeffreylovitz · 2020-06-11T16:08:26Z

src/commands/cmd_profile.c

+	* 1. AST
+	* 2. Execution plan (if any)
+	* 3. Execution type (query, index operation, invalid execution due to error)
+	* 4. Was the retrived items above where cached or not    */


"Whether these items were cached or not"

jeffreylovitz · 2020-06-11T16:11:15Z

src/commands/execution_ctx.c

+void ExecutionInformation_FromQuery(const char *query, ExecutionPlan **plan, AST **ast,
+									ExecutionType *exec_type, bool *cached) {


I feel like this function is overloaded and should probably be split into more discrete calls.

It currently:

Parses parameters

Attempts cache retrieval

Parses query, builds AST

Builds entire ExecutionPlan

Updates the cache

This flow is the reason I wrote this function. I don't think cmd_xxxxx should handle those tasks. We should have a designated objects/functions for this, and less objects and functions to handle in the cmd_xxxx handlers is better IMO.

I agree with Jeffrey this function is dealing with too much, we need to break it down to smaller more distinct units

jeffreylovitz · 2020-06-11T16:11:55Z

src/commands/cmd_query.c

+	 * 1. AST
+	 * 2. Execution plan (if any)
+	 * 3. Execution type (query, index operation, invalid execution due to error)
+	 * 4. Was the retrived items above where cached or not    */


Same comment as other command files.

jeffreylovitz · 2020-06-11T16:19:52Z

src/graph/graphcontext.c

+	/* Build the cache pool. The cache pool contains a cache for each thread in the thread pool, to avoid congestion.
+	 * Each thread is getting its cache by its thread id. */


I think it is unwise to maintain 1 cache per thread, and avoiding locks is an inadequate justification for this setup.

As a user I would be very confused to find my repeated queries having seemingly arbitrary cache misses, and after saturating all threads, we'll have N-1 redundant copies of an execution plan.

As an alternative, I think you should consider writing to the cache in a separate worker thread to avoid lock contention on insertion. For cache retrieval, either accept the minimal wait of lock acquisition or, if you really think that is unacceptable, consider solutions like pthread_mutex_timedlock or pthread_mutex_trylock that give up on cache retrieval.

jeffreylovitz

I feel that segment-based ExecutionPlan cloning is less efficient and safe than op-tree-base cloning, but that has been discussed.
I don't believe that cache information should be part of the result set statistics; it makes testing easier but requires client updates and has no benefit for users (especially because the multiple caches make the information non-deterministic).

swilly22

I've yet to review cache and cache_list files,
I think we should go over execution_plan clone online

swilly22 · 2020-06-16T07:19:51Z

src/resultset/resultset.c

+	buflen = sprintf(buff, "Cached execution: %s", set->stats.cached ? "true" : "false");
+	RedisModule_ReplyWithStringBuffer(ctx, (const char *)buff, buflen);


I was hoping the redismodule API would have ReplyWithBoolean but I don't it, I think we should switch from string 'true', 'false' to numeric: 1, 0.

swilly22 · 2020-06-16T07:24:19Z

src/graph/graphcontext.c

+// Returns thread id, with consdiration of redis main thread.
+static int get_thread_id() {
+	/* thpool_get_thread_id returns -1 if pthread_self isn't in the thread pool
+	 * most likely Redis main thread */
+	int thread_id = thpool_get_thread_id(_thpool, pthread_self());
+	thread_id += 1; // +1 to compensate for Redis main thread.
+	return thread_id;
+}


Please remove this function, it is only used once and I don't think graphcontext.c should have such a function in it, seems out of place.

This is used here:

// Return cache associated with graph context and current thread id. Cache *GraphContext_GetCache(const GraphContext *gc) { assert(gc); return gc->cache_pool[get_thread_id()]; }

I moved it to the function body.

Another approach is that we can make the caller send thread_id, which un-abstracts how caches are handles and store Cache *GraphContext_GetCache(const GraphContext *gc, int thread_id)
I prefer to have it internal
thoughts?

swilly22 · 2020-06-16T07:25:30Z

src/graph/graphcontext.c

+		for(uint i = 0; i < len; i++) {
+			Cache_Free(gc->cache_pool[i]);
+		}


swilly22 · 2020-06-16T07:26:35Z

src/execution_plan/optimizations/optimizer.c

@@ -51,7 +51,8 @@ void _optimizePlan(ExecutionPlan *plan) {
 void optimizePlan(ExecutionPlan *plan) {
 	/* Handle UNION of execution plans. */
 	if(plan->is_union) {
-		for(uint i = 0; i < plan->segment_count; i++) _optimizePlan(plan->segments[i]);


Why was segment_count removed?

segments was a simple heap-allocated array. I changed to arr.h array so we can get the length from it.
I changed it to have a uniform approach to arrays in the execution plan struct since connected componenets was arr.h array. It seems better and cleaner to code this way.

swilly22 · 2020-06-16T07:27:56Z

src/execution_plan/ops/shared/create_functions.c

@@ -127,7 +127,7 @@ PendingCreations NewPendingCreationsContainer(NodeCreateCtx *nodes, EdgeCreateCt
 	pending.created_edges = array_new(Edge *, 0);
 	pending.node_properties = array_new(PendingProperties *, 0);
 	pending.edge_properties = array_new(PendingProperties *, 0);
-	pending.stats = QueryCtx_GetResultSetStatistics();
+	pending.stats = NULL;


After the new modifications, the result set is created after the execution plan is built. In order to not get NULL from QueryCtx_GetResultSetStatistics() in the operation building phase, I moved the call to the init phase, where everything is built and valid.

swilly22 · 2020-06-16T11:43:34Z

tests/unit/test_execution_plan_clone.cpp

+	}
+};
+
+TEST_F(ExecutionPlanCloneTest, TestCreateClause) {


swilly22 · 2020-06-16T11:44:27Z

tests/unit/test_execution_plan_clone.cpp

+	queries = array_append(queries, "CREATE ()-[e:E]->() RETURN e");
+	queries = array_append(queries, "CREATE ()-[e:E {val:1}]->()");
+	uint query_count = array_len(queries);
+	for(uint i = 0; i < query_count; i++) {


I like the way these test are constructed, I think you can factorise out this loop into a function

swilly22 · 2020-06-16T11:56:26Z

tests/unit/test_cache.cpp

+class CacheTest:
+	public ::testing::Test {
+  protected:
+	static void SetUpTestCase() {// Use the malloc family for allocations


{// space between { and //

swilly22 · 2020-06-16T12:05:01Z

tests/unit/test_cache.cpp

+};
+
+TEST_F(CacheTest, ExecutionPlanCache) {
+	Cache *cache = Cache_New(3, (listValueFreeFunc)ExecutionPlan_Free);


listValueFreeFunc should be renamed, if this function is used to free the cached item it should be named accordingly without revealing the underline implementation.

swilly22 · 2020-06-16T12:06:03Z

tests/unit/test_cache.cpp

+	Cache_SetValue(cache, query4, ep4);
+	ASSERT_EQ(ep4, Cache_GetValue(cache, query4));
+
+	// Verify that oldest entry is not exists - queue is [ 4 | 3 | 2 ].


Should is be do ?

swilly22

a few comments regarding cache

swilly22 · 2020-06-18T07:54:01Z

src/util/cache/cache.h

+ *                 supply an appropriate function to avoid double resource releasing
+ * @retval cache pointer - Initialized empty cache.
+ */
+Cache *Cache_New(uint size, listValueFreeFunc freeCB);


listValueFreeFunc should be renamed, CacheItemFreeFunc

swilly22 · 2020-06-18T07:56:02Z

src/util/cache/cache.h

+ * @param  *key: Key to look for (bytes array).
+ * @retval  pointer with the cached answer, NULL if the key isn't cached.
+ */
+void *Cache_GetValue(Cache *cache, const char *key);


I'm not expecting Cache_GetValue to modify the cache in any way, in which case, pass cache as const

swilly22 · 2020-06-18T07:56:50Z

src/util/cache/cache.h

+void *Cache_GetValue(Cache *cache, const char *key);
+
+/**
+ * @brief  Stores value under key within the cache.


Should note cache eviction if there's no room in the cache.

swilly22 · 2020-06-18T07:57:14Z

src/util/cache/cache.h

+void Cache_SetValue(Cache *cache, const char *key, void *value);
+
+/**
+ * @brief  Destroy a cache


and free all stored items.

swilly22 · 2020-06-18T07:58:30Z

src/util/cache/cache.c

+#include "../rmalloc.h"
+
+/**
+ * @brief  Hash a query using XXHASH into a 64 bit.


Change Hash a query to Hash a key

query is to specific, we should keep this as general as possible

swilly22 · 2020-06-18T08:05:18Z

src/util/cache/cache_list.h

+typedef struct CacheListNode_t {
+	void *value;
+	uint64_t hashval;
+	struct CacheListNode_t *prev;
+	struct CacheListNode_t *next;
+} CacheListNode;
+
+typedef struct {
+	CacheListNode *buffer;
+	CacheListNode *head;
+	CacheListNode *tail;
+	uint buffer_len;
+	uint buffer_cap;
+	listValueFreeFunc ValueFree;
+} CacheList;


Please add documentation for these structs

swilly22 · 2020-06-18T08:07:01Z

src/util/cache/cache_list.h

+CacheListNode *CacheList_RemoveTail(CacheList *list);
+
+// Populate a new node and add it as the head of the list.
+CacheListNode *CacheList_NewNode(CacheList *list, CacheListNode *node, uint64_t hashval,


Please rename this function, I wouldn't expect NewNode to be passed in CacheListNode *node

swilly22 · 2020-06-18T08:07:36Z

src/util/cache/cache_list.h

+CacheList *CacheList_New(uint size, listValueFreeFunc freeCB);
+
+// Return true if the cache list is at capacity.
+bool CacheList_IsFull(CacheList *list);


list should be passed as const

swilly22 · 2020-06-18T08:09:48Z

src/util/cache/cache_list.c

+}
+
+CacheListNode *CacheList_RemoveTail(CacheList *list) {
+	// We can only get here on a filled list.


Consider adding an assertion validating this statement.

swilly22 · 2020-06-18T08:12:12Z

src/util/cache/cache_list.c

+	/* TODO consider replacing this with a rax callback that frees node->value.
+	 * This may not be possible while remaining data-agnostic,
+	 * as we need the callback to look like ExecutionPlan_Free(node->value)
+	 * OTOH, maybe possible if values in rax are actual plans, but then lookups are less valuable.
+	 */


This comment deals with a specific case, either make it generic or remove it.

swilly22 · 2020-06-18T18:12:42Z

src/execution_plan/execution_plan_clone.h

+#include "execution_plan.h"
+
+/* Clones an execution plan */
+ExecutionPlan *ExecutionPlan_Clone(const ExecutionPlan *plan);


Missing empty new line at the very end of this file

swilly22 · 2020-06-18T19:38:59Z

src/commands/cmd_query.c

+	ExecutionType exec_type = exec_ctx.exec_type;
 	if(exec_type == EXECUTION_TYPE_INVALID) goto cleanup;


Do we need to check exec_type, isn't the test above line 77 is enough?

swilly22 · 2020-06-18T19:42:29Z

src/commands/execution_ctx.c

+	GraphContext *gc = QueryCtx_GetGraphCtx();
+	Cache *cache = GraphContext_GetCache(gc);
+	// Check the cache to see if we already have a cached context for this query.
+	ExecutionCtx *cached_exec_ctx = _ExecutionCtx_FromCache(cache, query_string, params_parse_result);


It doesn't make sense to pass params_parse_result to _ExecutionCtx_FromCache I think AST_SetParamsParseResult(cached_exec_ctx->ast, params_parse_result); should be called in this function if we had a cache hit

swilly22 · 2020-06-18T19:43:46Z

src/commands/execution_ctx.c

+	// No cached execution plan, try to parse the query.
+	AST *ast = _ExecutionCtx_ParseAST(query_string, params_parse_result);
+	// Invalid query, return invalid execution context.
+	if(!ast) return invalid_ctx;


Don't we need to free params_parse_result ?

taken care in

static AST *_ExecutionCtx_ParseAST(const char *query_string, cypher_parse_result_t *params_parse_result) { cypher_parse_result_t *query_parse_result = parse_query(query_string); // If no output from the parser, the query is not valid. if(!query_parse_result) { parse_result_free(params_parse_result); return NULL; } // Prepare the constructed AST. AST *ast = AST_Build(query_parse_result); // Set parameters parse result in the execution ast. AST_SetParamsParseResult(ast, params_parse_result); return ast; }

swilly22 · 2020-06-18T19:45:20Z

src/commands/execution_ctx.c

-		// Clone execution plan.
-		*plan = ExecutionPlan_Clone(exec_plan_template);
+		Cache_SetValue(cache, query_string, exec_ctx_to_cache);
+		// Clone execution plan and plan to be used in the execution.


This comment needs some rephrasing

swilly22 · 2020-06-18T19:50:53Z

src/commands/execution_ctx.c

+
+ExecutionCtx ExecutionCtx_FromQuery(const char *query) {
+	// Have an invalid ctx for errors.
+	ExecutionCtx invalid_ctx = {0};


Please be specific with the values of an invalid plan, setting each attribute accordingly.

swilly22 · 2020-06-18T19:53:46Z

src/commands/execution_ctx.c

 	}
-	return exec_type;
+	ExecutionCtx ctx = {.ast = ast, .plan = plan, .exec_type = exec_type, .cached = false};
+	return ctx;
 }

 void ExecutionCtx_Free(ExecutionCtx *ctx) {


Who's calling this function?

Cache create in graph context, as this is the cache value free callback

…Graph into exec_plan_cache

* Update op_node_by_id_seek.c Fix, wrong variable assignment. * Update value.c removed.boolean switch * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) * Fixed compiler typo (#1097) Paragraph on OSX build should read "CXX" instead of "CPP". * Union bugfixes (#1052) * Improve scoping rules in validating UNION queries * Bugfix in uniquing column containing both nodes and edges * Add flow tests * PR fixes * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments * throw runtime-error on missing query parameters (#1100) * removed query parameters annotations (#1101) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) * changed RediSearch version to 1.8 (#1103) * changed RediSearch version to 1.8 * added flag for redisearch GC * moved env var setting to memcheck script * Update memcheck.sh * fixed misplaced env var setting * fixed bad command format * flag in circle ci instead of makefile Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * docker file for centos (#1104) * Skip NULL-valued properties in bulk loader (#1108) * Skip NULL-valued properties in bulk loader * Address PR comment, add missing logic in edge procesing * RED-4187: update RS version to 5.6.0 and run on Centos os on CI process (#1112) * Perform one-time transpose of traversed matrices (#1111) * Autoformat * WIP * No-op transpose nodes, working with memory leaks * WIP freeing and simplifying * WIP * Fix false positives on leak checking * Autoformat unit test file * Replace transpose op nodes with operands * Prune unused eval conditions, improve function logic * Fix unit tests * Partially address PR comments * PR fixes continued * PR fixes * Add handling for transpose operations in ApplyTranspose * Fix disconnect sequence in ApplyTranspose on operations Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Omp thread configuration (#1118) * Expose OMP_THREAD_COUNT as a module-level configuration * Update documentation * Improve comments * Refactor logic for module-level configuration * Fix PR comments * Enforce that module args are key-value pairs * PR fixes * PR fixes * redisearch 1.8.1 (#1121) * add doc action * remove internal folders * Add configurations to menu * Update deploy-docs.yaml (#1126) * Update deploy-docs.yaml (#1131) * Null PR for triggering docs (#1132) * Remove deploy-docs (moved to action) (#1140) * update min_redis_pack_version to 5.4.14 (#1142) * Transposed relations (#877) * maintain transposed matrices * Updated unit-tests * Post-rebase fixes * Revert FetchOperands changes * WIP * Unit test fixes * Simplify transpose matrix assignment * Add configuration param * Access config global to check for transposed matrices * Update documentation * Update graph logic to handle the absence of transposed matrices * Fix unit tests * Exit with error on unhandled parameter * Add flow test * Simplify fetch operands logic * Add abstraction layer to check for transposed relations * Partially address PR comments * Update FetchOperands logic * Address PR comments Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update index.md (#1143) * Exec plan cache (#1117) * added single threaded LRU cache * changed LRU logic. WIP after design review * fixed PR comments * removed DS_Store * changed pr comments for test priority queue * fixed priority_queue.h and linked_list.h comments. wip * fixed linked_list.c comments. wip * wip * Revert change to queue item sizing * WIP * wip * Remove linear insertion flag, simplify linked list * Refactor cache data structure implementations * Start adding logic for populating cache * added cache size config param * added cache API for graph context * did some clean ups * clone logic * done refactoring. unit tests pass * unit tests pass * wip * wip * test suit pass * test suit pass * fixed some memory leaks * wip * added cache tests * review ready * fixed PR comments * fixed PR comments * fixed PR comments * after rebase * fixed PR comments * fixing memory leaks * trying to avoid race Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * memceck race handling (#1152) * removed time.sleep from tests teardown * added async delete config * fixed config and makefile * moved memcheck to compiler flag * Avoid flushing matrices by maintaining separate transpose edge arrays (#1148) * Avoid flushing matrices by maintaining separate edge arrays in transposes * Properly update transpose matrices on single-edge deletion Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * version bump * fixed parameterized index scan (#1157) * added params support for indexed array lookup (#1159) * removed RedisModule_ReplyWithError from ast validations (#1160) * removed RedisModule_ReplyWithError from ast validations * removed char** reason from functions * refactored query_ctx for varargs Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * added params support to id seek (#1164) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments * validate graph schema is encoded (#1168) * validate graph schema is encoded * fixed PR comment Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Resolve race in accessing/updating attribute maps (#1165) * Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Move index iterator construction to first Consume call (#1169) * Move index iterator construction to first Consume call * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Christoph Zimmermann <40485189+chrisAtRedis@users.noreply.github.com> Co-authored-by: Omri Ben-Gidon <47712555+omrib1@users.noreply.github.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Itamar Haber <itamar@redislabs.com> Co-authored-by: swilly22 <roi@redislabs.com>

* Update op_node_by_id_seek.c Fix, wrong variable assignment. * Update value.c removed.boolean switch * Add suppression for erroneous leak reports after DEBUG RELOAD (#1094) * Fixed compiler typo (#1097) Paragraph on OSX build should read "CXX" instead of "CPP". * Union bugfixes (#1052) * Improve scoping rules in validating UNION queries * Bugfix in uniquing column containing both nodes and edges * Add flow tests * PR fixes * PR fixes Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Bidirectional expand into (#1047) * wip * Remove unnecessary iterator * WIP * Fix bidirectional ExpandInto, shared edge populating logic * PR fixes * Post-rebase fixes * Fix PR comments * throw runtime-error on missing query parameters (#1100) * removed query parameters annotations (#1101) * do not propagate transpose effect when introducing a transpose operation, maintain expression structure (#1102) * changed RediSearch version to 1.8 (#1103) * changed RediSearch version to 1.8 * added flag for redisearch GC * moved env var setting to memcheck script * Update memcheck.sh * fixed misplaced env var setting * fixed bad command format * flag in circle ci instead of makefile Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * docker file for centos (#1104) * Skip NULL-valued properties in bulk loader (#1108) * Skip NULL-valued properties in bulk loader * Address PR comment, add missing logic in edge procesing * RED-4187: update RS version to 5.6.0 and run on Centos os on CI process (#1112) * Perform one-time transpose of traversed matrices (#1111) * Autoformat * WIP * No-op transpose nodes, working with memory leaks * WIP freeing and simplifying * WIP * Fix false positives on leak checking * Autoformat unit test file * Replace transpose op nodes with operands * Prune unused eval conditions, improve function logic * Fix unit tests * Partially address PR comments * PR fixes continued * PR fixes * Add handling for transpose operations in ApplyTranspose * Fix disconnect sequence in ApplyTranspose on operations Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Omp thread configuration (#1118) * Expose OMP_THREAD_COUNT as a module-level configuration * Update documentation * Improve comments * Refactor logic for module-level configuration * Fix PR comments * Enforce that module args are key-value pairs * PR fixes * PR fixes * redisearch 1.8.1 (#1121) * add doc action * remove internal folders * Add configurations to menu * Update deploy-docs.yaml (#1126) * Update deploy-docs.yaml (#1131) * Null PR for triggering docs (#1132) * Remove deploy-docs (moved to action) (#1140) * update min_redis_pack_version to 5.4.14 (#1142) * Transposed relations (#877) * maintain transposed matrices * Updated unit-tests * Post-rebase fixes * Revert FetchOperands changes * WIP * Unit test fixes * Simplify transpose matrix assignment * Add configuration param * Access config global to check for transposed matrices * Update documentation * Update graph logic to handle the absence of transposed matrices * Fix unit tests * Exit with error on unhandled parameter * Add flow test * Simplify fetch operands logic * Add abstraction layer to check for transposed relations * Partially address PR comments * Update FetchOperands logic * Address PR comments Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * Update index.md (#1143) * Exec plan cache (#1117) * added single threaded LRU cache * changed LRU logic. WIP after design review * fixed PR comments * removed DS_Store * changed pr comments for test priority queue * fixed priority_queue.h and linked_list.h comments. wip * fixed linked_list.c comments. wip * wip * Revert change to queue item sizing * WIP * wip * Remove linear insertion flag, simplify linked list * Refactor cache data structure implementations * Start adding logic for populating cache * added cache size config param * added cache API for graph context * did some clean ups * clone logic * done refactoring. unit tests pass * unit tests pass * wip * wip * test suit pass * test suit pass * fixed some memory leaks * wip * added cache tests * review ready * fixed PR comments * fixed PR comments * fixed PR comments * after rebase * fixed PR comments * fixing memory leaks * trying to avoid race Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> * memceck race handling (#1152) * removed time.sleep from tests teardown * added async delete config * fixed config and makefile * moved memcheck to compiler flag * Avoid flushing matrices by maintaining separate transpose edge arrays (#1148) * Avoid flushing matrices by maintaining separate edge arrays in transposes * Properly update transpose matrices on single-edge deletion Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * fixed parameterized index scan (#1157) * added params support for indexed array lookup (#1159) * removed RedisModule_ReplyWithError from ast validations (#1160) * removed RedisModule_ReplyWithError from ast validations * removed char** reason from functions * refactored query_ctx for varargs Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * added params support to id seek (#1164) * added params support to id seek * reduce to scalar with runtime * fixed PR comments * fixed PR comments * validate graph schema is encoded (#1168) * validate graph schema is encoded * fixed PR comment Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Resolve race in accessing/updating attribute maps (#1165) * Protect critical region of reading/updating GraphContext attributes * Improve lock coverage * Don't heap-alloc attribute ID values * Change to QueryCtx lock check * Bulk insert deadlock fix * Remove FindOrAddAttribute calls from critical region * Revert changes to QueryCtx * Revert changes to Graph * Introduce new rwlock for attribute mapping * Only lookup property ID once * Fix unit tests * Fix possible duplicate entry * Always retrieve attribute value in critical region * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Move index iterator construction to first Consume call (#1169) * Move index iterator construction to first Consume call * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * fix graph creation example (#1171) * fix graph creation example * fixed errors in example * single flow (#1177) * added explain and profile options as invalid options in redisgraph (#1184) * added explain and profile options as invalid options in redisgraph query string * fixed PR comments * moved tests * Update Dockerfile (#1188) * Adding redis cloud pro to quick start Please hold back merging this PR. Thanks * fix typo in anker * enable search GC (#1194) * aggregated slowlog should maintain original timestamps (#1199) * correct link to redis cloud * Aumation auth moved to be token based (#1192) * Aumation auth moved to be token based * Update config.yml * Update config.yml * relay on search replace add functionality to delete existing documents (#1226) * line length 80 (#1227) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Emit compile time error on creation of undirected edges (#1212) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * redisearch 1.8.2 (#1229) * redisearch 1.8.2 * linked search cleanup function * add GTM (#1239) * Update index on change (#1225) * WIP * Fix compile errors * Only update indexes when necessary * refined update entity update eval * WIP * WIP * Compilation fixes, edge updates * Add freeing logic * Update comments * some minor changes to op_update * Address PR comments * Address PR comments * always get updated node label id Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: DvirDukhan <dvir@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Christoph Zimmermann <40485189+chrisAtRedis@users.noreply.github.com> Co-authored-by: Omri Ben-Gidon <47712555+omrib1@users.noreply.github.com> Co-authored-by: Guy Korland <gkorland@gmail.com> Co-authored-by: Itamar Haber <itamar@redislabs.com> Co-authored-by: Martin Rauscher <hades32@gmail.com> Co-authored-by: Pieter Cailliau <pieter.cailliau@gmail.com>

* added single threaded LRU cache * changed LRU logic. WIP after design review * fixed PR comments * removed DS_Store * changed pr comments for test priority queue * fixed priority_queue.h and linked_list.h comments. wip * fixed linked_list.c comments. wip * wip * Revert change to queue item sizing * WIP * wip * Remove linear insertion flag, simplify linked list * Refactor cache data structure implementations * Start adding logic for populating cache * added cache size config param * added cache API for graph context * did some clean ups * clone logic * done refactoring. unit tests pass * unit tests pass * wip * wip * test suit pass * test suit pass * fixed some memory leaks * wip * added cache tests * review ready * fixed PR comments * fixed PR comments * fixed PR comments * after rebase * fixed PR comments * fixing memory leaks * trying to avoid race Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>

DvirDukhan force-pushed the exec_plan_cache branch from 0eb2268 to d9fe843 Compare June 9, 2020 16:28

DvirDukhan closed this Jun 10, 2020

DvirDukhan reopened this Jun 10, 2020

DvirDukhan force-pushed the exec_plan_cache branch from 3d67080 to dab8aa9 Compare June 10, 2020 23:21

DvirDukhan marked this pull request as ready for review June 10, 2020 23:38

DvirDukhan requested review from swilly22 and jeffreylovitz and removed request for swilly22 June 10, 2020 23:38

jeffreylovitz suggested changes Jun 11, 2020

View reviewed changes

jeffreylovitz previously approved these changes Jun 15, 2020

View reviewed changes

swilly22 requested changes Jun 16, 2020

View reviewed changes

DvirDukhan dismissed jeffreylovitz’s stale review via e8c9740 June 16, 2020 21:08

swilly22 requested changes Jun 18, 2020

View reviewed changes

DvirDukhan and others added 16 commits June 18, 2020 20:19

added single threaded LRU cache

cddd07c

changed LRU logic. WIP after design review

f391075

fixed PR comments

b9b98af

removed DS_Store

0bb0d72

changed pr comments for test priority queue

2886af7

fixed priority_queue.h and linked_list.h comments. wip

fb4ef43

fixed linked_list.c comments. wip

aa98e04

wip

3c7937a

Revert change to queue item sizing

67d081f

WIP

414c4f1

wip

a7cb729

Remove linear insertion flag, simplify linked list

1197ab4

Refactor cache data structure implementations

57448a8

Start adding logic for populating cache

ec79a80

added cache size config param

a129cd4

added cache API for graph context

b5d0aff

DvirDukhan added 16 commits June 18, 2020 20:42

did some clean ups

80de52c

clone logic

0fbe2fc

done refactoring. unit tests pass

843a7b3

unit tests pass

eebc699

wip

e7b16f9

wip

0224219

test suit pass

8694fdc

test suit pass

9062dc9

fixed some memory leaks

f50321d

wip

48dc56a

added cache tests

1a41a4b

review ready

6076e5d

fixed PR comments

c852dea

fixed PR comments

135ef75

fixed PR comments

c669f35

after rebase

993c870

DvirDukhan force-pushed the exec_plan_cache branch from c889dfb to 993c870 Compare June 18, 2020 17:51

swilly22 requested changes Jun 18, 2020

View reviewed changes

fixed PR comments

b5ae32c

swilly22 previously approved these changes Jun 18, 2020

View reviewed changes

fixing memory leaks

6837f56

DvirDukhan dismissed swilly22’s stale review via 6837f56 June 18, 2020 21:35

DvirDukhan added 2 commits June 19, 2020 13:35

trying to avoid race

c5624da

Merge branch 'exec_plan_cache' of https://github.com/RedisGraph/Redis…

2c46f7c

…Graph into exec_plan_cache

swilly22 approved these changes Jun 19, 2020

View reviewed changes

swilly22 merged commit f8bb292 into master Jun 19, 2020

swilly22 deleted the exec_plan_cache branch June 19, 2020 13:23

		void ExecutionInformation_FromQuery(const char query, ExecutionPlan plan, AST *ast,
		ExecutionType exec_type, bool cached) {

		/* Build the cache pool. The cache pool contains a cache for each thread in the thread pool, to avoid congestion.
		* Each thread is getting its cache by its thread id. */

		buflen = sprintf(buff, "Cached execution: %s", set->stats.cached ? "true" : "false");
		RedisModule_ReplyWithStringBuffer(ctx, (const char *)buff, buflen);

		ExecutionType exec_type = exec_ctx.exec_type;
		if(exec_type == EXECUTION_TYPE_INVALID) goto cleanup;

Exec plan cache #1117

Exec plan cache #1117

Conversation

DvirDukhan commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreylovitz left a comment

Choose a reason for hiding this comment

swilly22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swilly22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment