Perform one-time transpose of traversed matrices #1111

jeffreylovitz · 2020-06-01T17:25:54Z

As discussed in #1058, this PR reintroduces the optimization to transpose matrices that require it once rather than per-traversal. This greatly increases performance of dest->src traversals.

With this change, AlgebraicExpression trees will no longer have any transpose operation nodes after the optimization stage. As such, all the code paths that handle these operations are currently unused, but have largely been left as-is since they may prove useful again in the future.

tests/unit/test_algebraic_expression.cpp

swilly22 · 2020-06-02T07:01:39Z

tests/unit/test_algebraic_expression.cpp

+		// Can't compare matrix pointers, as transpose ops will have rewritten them
+		// ASSERT_EQ(a->operand.matrix, b->operand.matrix);


Can we at least check operand's domains?
it is important to verify the actual operands.

swilly22 · 2020-06-02T07:04:02Z

tests/Makefile

@@ -37,6 +37,7 @@ tck:
 	### Cypher Technology Compatibility Kit (TCK)
 	@$(MAKE) -C tck TEST_ARGS="$(TEST_ARGS)"

+memcheck: export RS_GLOBAL_DTORS = 1


What's that?

The new RediSearch version has some false positive leaks that require this environment variable to be set. This is a fix on 9f2afd3.

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

src/arithmetic/algebraic_expression.h

-        } operand;
+		struct {
+			bool diagonal;          // Diagonal matrix.
+			bool should_free;       // If the matrix is scoped to this expression, it should be freed with it.


What do you think of renaming to free or bfree ?

swilly22 · 2020-06-02T07:10:10Z

src/arithmetic/algebraic_expression.h

+void AlgebraicExpression_Initialize(
+	AlgebraicExpression **exp,      // Expression to prep for evaluation.
+	GrB_Matrix filter_matrix       // Filter matrix for leftmost multiplication.
 );


Missing comment explaining the purpose of this function,
Also each parameter comment should be aligned,
Lastly algebraic_expression.h is an API for describing generic algebraic expression,
a parameter such as filter_matrix is too specific, the filter matrix f should be appended via the multiply left function.

Function deleted.

swilly22 · 2020-06-02T12:15:32Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

@@ -24,20 +25,20 @@ static void _AlgebraicExpression_CollectOperands(AlgebraicExpression *root,
 	switch(root->type) {
 	case AL_OPERAND:
 		*operands = array_append(*operands, AlgebraicExpression_Clone(root));
+		root->operand.should_free = false; // The caller is the new owner of this operand.


Can you please explain this addition?

This was necessary because transposition was being performed before the FlattenMultiplications optimization, which frees and replaces the original expression. This caused constructed matrices to be freed prematurely, and left invalid pointers in the new expression.

Since the transpose optimizations are now done in two parts, this line will not currently cause any changes, but I think logically it belongs!

swilly22 · 2020-06-02T12:19:42Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+	GrB_Index nrows;
+	GrB_Matrix replacement;
+	// Create a new empty matrix with the type and dimensions of the original.
+	GrB_Matrix_nrows(&nrows, operand->operand.matrix);


Just to be more generic please call GrB_Matrix_ncols and use both nrows and ncols

swilly22 · 2020-06-02T12:19:58Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+	// Create a new empty matrix with the type and dimensions of the original.
+	GrB_Matrix_nrows(&nrows, operand->operand.matrix);
+	GxB_Matrix_type(&type, operand->operand.matrix);
+	GrB_Matrix_new(&replacement, type, nrows, nrows);


Assert success.

swilly22 · 2020-06-02T12:22:09Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

@@ -328,17 +339,19 @@ static void _Pushdown_TransposeExp
 		_Pushdown_TransposeOperation(exp);
 		break;
 	case AL_OPERAND:
-		_Pushdown_TransposeOperand(exp);
+		_AlgebraicExpression_TransposeOperand(exp);


I don't think PushDown should perform transpose,
this function should simply relocate nodes within the tree.

there should be another optimisation function following pushdown which replaces transpose operations acting directly on operands with just the transposed operand.

swilly22 · 2020-06-02T12:24:18Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+ * Then transpose the operands and replace the operation nodes with them.
+ * Once this optimization is applied, there shouldn't be any transpose operation nodes
+ * in the tree, and individual operands will be transposed matrices where appropriate.


Please break this into two different optimisations, one _AlgebraicExpression_PushDownTranspose which simply relocates nodes

And another optimisations function which looks for transpose nodes applied directly to operands and performs the transpose, replacing the transpose operation node and the operand node with just a single transposed operand node.

jeffreylovitz · 2020-06-02T16:42:45Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+	// We need to fetch operand matrices now, as the PerformTranspose optimization will utilize them.
+	_AlgebraicExpression_FetchOperands(*exp, QueryCtx_GetGraphCtx(), QueryCtx_GetGraph());


@swilly22 Fetching operands here is a questionable choice, and I welcome your thoughts! We do require the matrices before the next call - the optimization does not make sense otherwise.

The alternative location for this call would be in ConditionalTraverse and ExpandInto before they call this function, but that would require changing this to an exposed interface and does not really seem preferable to me.

We can also just put this at the start of this function.

I don't mind leaving _AlgebraicExpression_FetchOperands here, but let's move it to the top of this function, as the first "optimisation" performed.

src/arithmetic/algebraic_expression/algebraic_expression.c

swilly22 · 2020-06-02T19:56:02Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+//------------------------------------------------------------------------------
+// Transpose an operand matrix and update the expression accordingly.
+static void _AlgebraicExpression_TransposeOperand(AlgebraicExpression *operand) {
+	if(operand->operand.diagonal == true) return;  // No need to transpose diagonal matrix.


For good measure perform swap of src and dest domains.

swilly22 · 2020-06-02T19:59:22Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+ *                (*)
+ *           (B')     (A')
+ */
+static void _AlgebraicExpression_PerformTranspose(AlgebraicExpression *root) {


General comment,
Do you know if we're canceling expressions such as T(T(A)) this should be simply A

Yes, that cancellation still occurs as part of the push-down optimization (_Pushdown_TransposeTranspose).

swilly22 · 2020-06-02T20:02:00Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+	// We need to fetch operand matrices now, as the PerformTranspose optimization will utilize them.
+	_AlgebraicExpression_FetchOperands(*exp, QueryCtx_GetGraphCtx(), QueryCtx_GetGraph());


I don't mind leaving _AlgebraicExpression_FetchOperands here, but let's move it to the top of this function, as the first "optimisation" performed.

swilly22 · 2020-06-02T20:02:27Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+	_AlgebraicExpression_FetchOperands(*exp, QueryCtx_GetGraphCtx(), QueryCtx_GetGraph());
+
+	// Replace transpose operators with actual transposed operands.
+	_AlgebraicExpression_PerformTranspose(*exp);


Consider renaming to _AlgebraicExpression_ApplyTranspose

swilly22 · 2020-06-03T06:17:09Z

tests/unit/test_algebraic_expression.cpp

+			_AlgebraicExpression_FetchOperands(ae[i], gc, g);
+			AlgebraicExpression_Optimize(ae + i);


AlgebraicExpression_Optimize calls _AlgebraicExpression_FetchOperands
and so the call to _AlgebraicExpression_FetchOperands should be dropped.

swilly22 · 2020-06-03T06:24:56Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+		case AL_EXP_TRANSPOSE:
+			// Transpose operands will currently always have an operand child, but
+			// since evalution can perform transposition we don't need to assert here.
+			if(root->operation.children[0]->type != AL_OPERAND) break;


keep calling _AlgebraicExpression_ApplyTranspose recursively in the same manner as all other operations,
consider: T(T(A) * T(B))

After applying _AlgebraicExpression_PushDownTranspose, there are no transpose operations that do not have a single operand child. Your example at this point would be A * T(B).

Do you want this change made for consistency regardless?

Yes please.

swilly22 · 2020-06-03T06:26:44Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

@@ -24,6 +25,7 @@ static void _AlgebraicExpression_CollectOperands(AlgebraicExpression *root,
 	switch(root->type) {
 	case AL_OPERAND:
 		*operands = array_append(*operands, AlgebraicExpression_Clone(root));
+		root->operand.bfree = false; // The caller is the new owner of this operand.


Can you please explain to me why are we resetting bfree ?
Who's calling _AlgebraicExpression_CollectOperands and for what purpose?

_AlgebraicExpression_FlattenMultiplications is the sole caller of this function; it uses this function to collect n-ary operands under the same multiplication operation rather than having nested multiplication operations.

It frees the operations that have been made redundant, and as such could cause invalid frees of operands that have been migrated.

This is not a possible scenario currently, but logically it makes sense to reset bfree here.

swilly22 · 2020-06-03T06:30:20Z

src/arithmetic/algebraic_expression/algebraic_expression_eval.c

 		GrB_Descriptor_set(desc, GrB_INP0, GrB_TRAN);
 		left = CHILD_AT(left, 0);
 	}
 	A = left->operand.matrix;

 	if(right->type == AL_OPERATION) {
 		assert(right->operation.op == AL_EXP_TRANSPOSE);
+		if(desc == GrB_NULL) GrB_Descriptor_new(&desc);


A bit further down, if B is the identity matrix, and there's a transpose over it, we should reset the descriptor for GrB_INP1

swilly22 · 2020-06-03T06:33:16Z

src/arithmetic/algebraic_expression/algebraic_expression_eval.c

-	GrB_Descriptor_new(&desc);
+	GrB_Matrix inter = GrB_NULL;    // Intermediate matrix.
+	GrB_Descriptor desc = GrB_NULL; // Descriptor used for transposing operands (currently unused).
+	bool res_in_use = false;        // Can we use `res` for intermediate evaluation.


Move up to maintain line length order.

swilly22 · 2020-06-04T06:01:38Z

src/arithmetic/algebraic_expression/algebraic_expression_optimization.c

+			if(child->type == AL_OPERATION) {
+				if(child->operation.op == AL_EXP_TRANSPOSE) {
+					/* If the child op is a transpose, we do not need to transpose anything, as
+					 * T(T(A)) == A */
+					// Disconnect the intermediate transpose op and its child.
+					child = _AlgebraicExpression_OperationRemoveRightmostChild(root);
+					AlgebraicExpression *grandchild = _AlgebraicExpression_OperationRemoveRightmostChild(root);
+					// Replace the current op with the grandchild.
+					_AlgebraicExpression_InplaceRepurpose(root, grandchild);
+					// Free the disconnected intermediate operation.
+					AlgebraicExpression_Free(child);
+				} else {
+					// Continue seeking transpose ops.
+					_AlgebraicExpression_ApplyTranspose(child);
+				}
+				break;
+			}


What if grandchild is an operation? we need to continue recursing

* 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>

* 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>

jeffreylovitz added 10 commits May 28, 2020 12:24

Autoformat

ce3bcf4

WIP

4813350

No-op transpose nodes, working with memory leaks

fa5f7ee

WIP freeing and simplifying

b56063c

Fix false positives on leak checking

3ddd40a

WIP

07c4baf

Autoformat unit test file

0ed3fd3

Replace transpose op nodes with operands

c56ee37

Prune unused eval conditions, improve function logic

e6c3937

Fix unit tests

f486430

jeffreylovitz requested review from swilly22 and DvirDukhan June 1, 2020 17:39

jeffreylovitz self-assigned this Jun 1, 2020

jeffreylovitz added the enhancement label Jun 1, 2020

swilly22 requested changes Jun 2, 2020

View reviewed changes

jeffreylovitz force-pushed the one-time-transpose branch 2 times, most recently from 9b3f0af to a85adda Compare June 2, 2020 16:38

Partially address PR comments

c9bafb5

jeffreylovitz force-pushed the one-time-transpose branch from a85adda to c9bafb5 Compare June 2, 2020 16:39

jeffreylovitz commented Jun 2, 2020

View reviewed changes

swilly22 requested changes Jun 2, 2020

View reviewed changes

PR fixes continued

9b1a0fc

swilly22 requested changes Jun 3, 2020

View reviewed changes

jeffreylovitz and others added 2 commits June 3, 2020 15:29

PR fixes

e4d7a2a

Merge branch 'master' into one-time-transpose

cb6c054

swilly22 requested changes Jun 4, 2020

View reviewed changes

Add handling for transpose operations in ApplyTranspose

d295262

jeffreylovitz force-pushed the one-time-transpose branch from b0278a5 to d295262 Compare June 4, 2020 10:00

Fix disconnect sequence in ApplyTranspose on operations

a683433

jeffreylovitz force-pushed the one-time-transpose branch from df89a50 to a683433 Compare June 4, 2020 12:31

swilly22 approved these changes Jun 6, 2020

View reviewed changes

swilly22 merged commit 2e2fd7b into master Jun 6, 2020

swilly22 deleted the one-time-transpose branch June 6, 2020 12:12

jeffreylovitz mentioned this pull request Jun 10, 2020

Duplicated computation of op_conditional_traverse #1058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform one-time transpose of traversed matrices #1111

Perform one-time transpose of traversed matrices #1111

jeffreylovitz commented Jun 1, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

jeffreylovitz Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

jeffreylovitz Jun 2, 2020

swilly22 Jun 2, 2020

jeffreylovitz Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

jeffreylovitz Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

jeffreylovitz Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 2, 2020

swilly22 Jun 3, 2020

swilly22 Jun 3, 2020

jeffreylovitz Jun 3, 2020

swilly22 Jun 3, 2020

swilly22 Jun 3, 2020

jeffreylovitz Jun 3, 2020

swilly22 Jun 3, 2020

swilly22 Jun 3, 2020

swilly22 Jun 4, 2020

		// Can't compare matrix pointers, as transpose ops will have rewritten them
		// ASSERT_EQ(a->operand.matrix, b->operand.matrix);

		// We need to fetch operand matrices now, as the PerformTranspose optimization will utilize them.
		_AlgebraicExpression_FetchOperands(*exp, QueryCtx_GetGraphCtx(), QueryCtx_GetGraph());

		_AlgebraicExpression_FetchOperands(ae[i], gc, g);
		AlgebraicExpression_Optimize(ae + i);

Perform one-time transpose of traversed matrices #1111

Perform one-time transpose of traversed matrices #1111

Conversation

jeffreylovitz commented Jun 1, 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

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