make delete operation eager by AviAvni · Pull Request #3152 · RedisGraph/RedisGraph

AviAvni · 2023-08-06T11:20:57Z

codecov · 2023-08-06T11:53:36Z

Codecov Report

Patch coverage: 98.84% and project coverage change: -0.07% ⚠️

Comparison is base (b52fbc5) 90.87% compared to head (b1b7e22) 90.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3152      +/-   ##
==========================================
- Coverage   90.87%   90.80%   -0.07%     
==========================================
  Files         294      294              
  Lines       30286    30286              
==========================================
- Hits        27522    27502      -20     
- Misses       2764     2784      +20

Files Changed	Coverage Δ
src/graph/graphcontext.c	`91.90% <0.00%> (ø)`
src/undo_log/undo_log.c	`95.45% <98.27%> (+0.93%)`	⬆️
src/arithmetic/entity_funcs/entity_funcs.c	`97.56% <100.00%> (ø)`
src/ast/ast.c	`91.94% <100.00%> (+0.02%)`	⬆️
src/commands/cmd_constraint.c	`94.11% <100.00%> (-0.04%)`	⬇️
src/commands/cmd_query.c	`97.86% <100.00%> (ø)`
src/effects/effects.c	`99.17% <100.00%> (ø)`
src/execution_plan/ops/op_delete.c	`97.52% <100.00%> (+0.08%)`	⬆️
src/execution_plan/ops/shared/update_functions.c	`98.25% <100.00%> (ø)`
src/graph/entities/edge.c	`80.00% <100.00%> (+1.73%)`	⬆️
... and 6 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

swilly22

Nice work!

filipecosta90 · 2023-08-08T06:55:56Z

Automated performance analysis summary

This comment was automatically generated given there is performance data available.

In summary:

Detected a total of 15 stable tests between versions.
Detected a total of 4 improvements above the improvement water line.
Detected a total of 1 regressions bellow the regression water line 5.0.

You can check a comparison in detail via the grafana link

Comparison between master and eager-op-delete.

Time Period from 30 days ago. (environment used: oss-standalone)

Test Case	Baseline master (median obs. +- std.dev)	Comparison eager-op-delete (median obs. +- std.dev)	% change (higher-better)	Note
ENTITY_COUNT	49999 +- 0.0% (5 datapoints)	49998 +- nan% (1 datapoints)	-0.0%	-- no change --
EXPLICIT-EDGE-DELETION	100 +- 0.0% (5 datapoints)	50 +- nan% (1 datapoints)	-50.0%	REGRESSION
GRAPH500-SCALE_18-EF_16-1_HOP	33323 +- 0.0% (5 datapoints)	33324 +- nan% (1 datapoints)	0.0%	-- no change --
GRAPH500-SCALE_18-EF_16-1_HOP_MIXED_READ_65perc_WRITE_25perc_DEL_10perc	50 +- 0.0% (5 datapoints)	50 +- nan% (1 datapoints)	-0.0%	-- no change --
GRAPH500-SCALE_18-EF_16-1_HOP_MIXED_READ_75perc_WRITE_25perc	56 +- 2.6% (5 datapoints)	59 +- nan% (1 datapoints)	5.9%	IMPROVEMENT
GRAPH500-SCALE_18-EF_16-2_HOP	11111 +- 5.5% (5 datapoints)	11111 +- nan% (1 datapoints)	0.0%	waterline=5.5%. -- no change --
GRAPH500-SCALE_18-EF_16-2_HOP_MIXED_READ_75perc_WRITE_25perc	56 +- 3.2% (5 datapoints)	59 +- nan% (1 datapoints)	5.9%	IMPROVEMENT
GRAPH500-SCALE_18-EF_16-3_HOP	893 +- 0.4% (5 datapoints)	893 +- nan% (1 datapoints)	0.0%	-- no change --
GRAPH500-SCALE_18-EF_16-3_HOP_MIXED_READ_75perc_WRITE_25perc	53 +- 3.0% (5 datapoints)	56 +- nan% (1 datapoints)	5.6%	IMPROVEMENT
IMPLICIT-EDGE-DELETION	17 +- 0.0% (5 datapoints)	50 +- nan% (1 datapoints)	199.8%	IMPROVEMENT
INSPECT-EDGES	2500 +- 0.0% (5 datapoints)	2500 +- nan% (1 datapoints)	-0.0%	-- no change --
MIX_READ_WRITE	10000 +- 0.0% (5 datapoints)	10000 +- nan% (1 datapoints)	-0.0%	-- no change --
NODE-BATCH-DELETE	50 +- 0.0% (5 datapoints)	50 +- nan% (1 datapoints)	0.0%	-- no change --
NODE-INDEX-LOOKUP	33 +- 0.0% (5 datapoints)	33 +- nan% (1 datapoints)	0.0%	-- no change --
SORT_ENTITIES	100 +- 0.0% (5 datapoints)	100 +- nan% (1 datapoints)	-0.0%	-- no change --
UPDATE-BASELINE	24998 +- 0.0% (5 datapoints)	24998 +- nan% (1 datapoints)	0.0%	-- no change --
VARIABLE-LENGTH-FILTER	33323 +- 0.0% (5 datapoints)	33323 +- nan% (1 datapoints)	-0.0%	-- no change --
VARIABLE_LENGTH_EXPAND_INTO	100 +- 0.0% (5 datapoints)	100 +- nan% (1 datapoints)	-0.0%	-- no change --
allShortestPaths-10hop-100Kpaths	30 +- 6.1% (5 datapoints)	30 +- nan% (1 datapoints)	-0.0%	waterline=6.1%. -- no change --
allShortestPaths-4hop-10Kpaths	333 +- 3.9% (5 datapoints)	333 +- nan% (1 datapoints)	-0.0%	-- no change --

swilly22

Review in progress

swilly22 · 2023-08-10T13:56:36Z

        res = redis_graph.query("MATCH (n:Bar) RETURN count(n)")
        self.env.assertEquals(res.result_set[0][0], 0)
-
+


I'm missing tests which are trying to interact / return deleted entities.
e.g

MATCH (a) DELETE a RETURN a.v MATCH (a) WITH a, a AS b DELETE a RETURN b MATCH (a) DELETE a CALL P(a) ... ...

maybe we need to finish with #3085

Probably. Still I think the tests should be either written or copied over from that PR.
I feel like we might get into crashes if we allow users to access deleted entities.

swilly22 · 2023-08-14T05:38:13Z

        res = redis_graph.query("MATCH (n:Bar) RETURN count(n)")
        self.env.assertEquals(res.result_set[0][0], 0)
-
+


Probably. Still I think the tests should be either written or copied over from that PR.
I feel like we might get into crashes if we allow users to access deleted entities.

swilly22 · 2023-08-14T06:32:48Z

-	// perform additions
-	//--------------------------------------------------------------------------
+	s = (t == GrB_BOOL) ? GxB_ANY_PAIR_BOOL : GxB_ANY_PAIR_UINT64;
+	info = GrB_Matrix_eWiseAdd_Semiring(m, NULL, NULL, s, m, dp, NULL);


If you want to investigate performance I've got a hunch that GrB_Matrix_assign would yield better performance

* make delete operation eager * address review comments * address review * address review * fix leak * fix benchmark issue * fix benchmark * back to scalar * try fix perf * fix benchmark * try to improve rg_wait * lazy create undo_log and effect_buffer * change sanizaiter parallel * address review * address review * addres review * fix warning * perf

* Updated clang sanitizer (sanbox) version (#3153) * make delete operation eager (#3152) * make delete operation eager * address review comments * address review * address review * fix leak * fix benchmark issue * fix benchmark * back to scalar * try fix perf * fix benchmark * try to improve rg_wait * lazy create undo_log and effect_buffer * change sanizaiter parallel * address review * address review * addres review * fix warning * perf * conditional reserve adjust (#3154) * conditional reserve adjust * Update test_graph_merge.py * bump version 2.12.5 --------- Co-authored-by: Rafi Einstein <raffapen@outlook.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

make delete operation eager

77dd56e

swilly22 suggested changes Aug 7, 2023

View reviewed changes

address review comments

19e78d1

swilly22 previously approved these changes Aug 7, 2023

View reviewed changes

Comment thread src/undo_log/undo_log.c

address review

2c271c3

AviAvni dismissed swilly22’s stale review via 2c271c3 August 7, 2023 08:02

address review

7311305

swilly22 previously approved these changes Aug 7, 2023

View reviewed changes

fix leak

31c917c

AviAvni dismissed swilly22’s stale review via 31c917c August 7, 2023 08:50

Merge branch 'master' into eager-op-delete

80dbe0c

AviAvni added the action:run-benchmark Trigger CI benchmarks label Aug 8, 2023

AviAvni added 8 commits August 8, 2023 12:03

fix benchmark issue

e192111

fix benchmark

fa87031

back to scalar

e391be2

try fix perf

01bf97b

fix benchmark

9a06202

try to improve rg_wait

87bb687

lazy create undo_log and effect_buffer

0d33f86

change sanizaiter parallel

f2139a6

swilly22 suggested changes Aug 10, 2023

View reviewed changes

swilly22 suggested changes Aug 11, 2023

View reviewed changes

AviAvni added 2 commits August 13, 2023 13:31

address review

ff5b75d

address review

784cab4

swilly22 suggested changes Aug 14, 2023

View reviewed changes

Comment thread src/execution_plan/ops/op_delete.c

Comment thread src/execution_plan/ops/op_delete.c

Comment thread src/execution_plan/ops/op_delete.c Outdated

Comment thread src/execution_plan/ops/op_delete.c

addres review

0246109

AviAvni added 2 commits August 14, 2023 12:19

fix warning

c990352

perf

b1b7e22

swilly22 approved these changes Aug 14, 2023

View reviewed changes

swilly22 merged commit 8dc6b59 into master Aug 14, 2023

swilly22 deleted the eager-op-delete branch August 14, 2023 12:33

This was referenced Sep 27, 2023

Crash due to untrue assertion rel != -1 #3041

Closed

Crash due to untrue assertion rel != -1 FalkorDB/FalkorDB#410

Closed

Conversation

AviAvni commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

swilly22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filipecosta90 commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated performance analysis summary

Comparison between master and eager-op-delete.

Uh oh!

swilly22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swilly22 Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

AviAvni Aug 13, 2023

Choose a reason for hiding this comment

Uh oh!

swilly22 Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swilly22 Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swilly22 Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AviAvni commented Aug 6, 2023 •

edited

Loading

codecov bot commented Aug 6, 2023 •

edited

Loading

filipecosta90 commented Aug 8, 2023 •

edited

Loading