fix memtracker bugs during stress test on graphd and storaged #5276

codesigner · 2023-01-19T07:30:05Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

graphd

fix multiple not throw safe code path
- fix Value's setXXX, should set type only when set ptr succedd, during request deserialize;
- fix ObjectPool deadlock if throw on alloc memory
- fix Symbol & ExecutionContext's valueMap_, not thread-safe when multiple Executor run concurrently
fix scheduler
- fix AsyncMsgNotifyBasedScheduler's scheduling logic, add wait mechinism to guarantee Executors all finish

storaged

write operators: turn off memtracker on stroage's write path
read operators: fix uncaught execption & memory leak for storage's read Operators

others

remove noexcept of functions those need alloc memory;
add MemoryCheckOffGuard to MetaClient's functions, meta need little memory, but they are important, turn off check on all these functions.

Tests

graphd
- running days on limited memory, triger memory exceeded for most queries, no crashed;
storaged
- run through three test scenario, not crashed, in https://github.com/vesoft-inc/nebula-ent/issues/2221
  - python3 run.py stress run -scenario go.Go3StepEdge
  - python3 run.py stress run -scenario lookup.LookUp
  - python3 run.py stress run -scenario find_path.FindShortestPath

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

critical27

Well done~ Hope we could survive long term test...

…emoryTracker

yixinglu · 2023-01-29T06:38:34Z

src/common/memory/MemoryUtils.cpp

+      }
+      LOG(INFO) << "MemoryTracker set static ratio: " << limitRatio;
+    }
+  } else if (limitRatio == 2.0) {


best not to use equals to compare floating point data types

best not to use equals to compare floating point data types

OK, change to floating compare is better
I had considered it as a issue, but I tried in real run environment, it works as expected.

yixinglu · 2023-01-29T06:38:48Z

src/common/memory/MemoryUtils.cpp

+      MemoryStats::instance().updateLimit(0);
+    }
+    DLOG(INFO) << "MemoryTracker set to dynamic self adaptive";
+  } else if (limitRatio == 3.0) {


yixinglu · 2023-01-29T06:41:25Z

src/common/memory/MemoryUtils.cpp

+     * (== 3.0): Special value for disable memory_tracker;
+     *           limit is set to max
+     */
+    if (kCurrentLimitRatio_ != limitRatio) {


yixinglu · 2023-01-29T06:46:39Z

src/graph/scheduler/AsyncMsgNotifyBasedScheduler.cpp

+    folly::Future<Status> status = Status::OK();
+    {
+      memory::MemoryCheckGuard guard;
+      status = executor->execute();


plz NOTE: the execute is an async function, your memory check guard may be invalid when the tasks really run.

plz NOTE: the execute is an async function, your memory check guard may be invalid when the tasks really run.

This MemoryCheckGuard is only used by some Executor return a already satisfied future, like DedupExecutor::execute, Async future's MemoryCheckGuard is not here, they are declare inside, for example in StorageClientBase-inl.h:

return folly::collectAll(respFutures) .deferValue([this, requests = std::move(requests), totalLatencies, hosts]( std::vector<folly::Try<StatusOr<Response>>>&& resps) { // throw in MemoryCheckGuard verified memory::MemoryCheckGuard guard; StorageRpcResponse<Response> rpcRes

yixinglu · 2023-01-29T06:49:13Z

src/graph/scheduler/AsyncMsgNotifyBasedScheduler.cpp

+  ss << std::string(spaces, ' ') << formatPrettyId(executor) << std::endl;
+  for (auto depend : executor->depends()) {
+    appendExecutor(spaces + 1, depend, ss);
+  }


do the loop body executors need to be considered here?

do the loop body executors need to be considered here?

no, just print a original dependency tree.

yixinglu

LGTM

* fix memtracker bugs during stress test on graphd and storaged * fix lint * fix RocksEngine memory leak of raw pointer iter * add ENABLE_MEMORY_TRACKER build option & support adaptive limit for MemoryTracker * delete debug log * refine log * refine log * fix build * refine error log * print warning if memtracker is off * fix rocksdb leak by turn off memcheck * refine synamic-self-adaptive * fix cmake check * minor * minor * minor * minor * minor * refine double equel compare

* optimize match node label (#5176) * revert strange return (#5183) Co-authored-by: Sophie <[email protected]> * fix stderr save error log (#5188) * fix processor_test timeout (#5180) * fix processor_test timeout * ... Co-authored-by: Yee <[email protected]> Co-authored-by: Sophie <[email protected]> * fix error code (#5186) * rename the "test" space to "ngdata". (#5197) * rename the "test" space to "ngdata". * add ngdata * Revise the usages of FATAL, DFATAL, LOG, DLOG. (#5181) * Revise the usages of FATAL, DFATAL, LOG, DLOG. * fix. * revise dfatal. * Meta upgrade (#5174) * Meta upgrade remove all fulltext index when upgrade from V3 to V3_4 because of refacting of fulltext index * fix bug Co-authored-by: Sophie <[email protected]> * Fix pattern expression with same edge variable (#5192) * Fix pattern expression with same edge variable add tck fmt * add tck * Fix memory leak, remove toss gflag (#5204) * remove toss gflag * fix memory leak * loose wait job finish time Co-authored-by: Sophie <[email protected]> * Add max_sessions_per_ip_per_user to default config file (#5207) * minor bug for adminTaskManager (#5195) * modify jobmanager ut (#5175) * modify jobmanager ut * add expired ut * avoid recover expired job * add ut * address review * move status Co-authored-by: Sophie <[email protected]> * Add more match test cases on paths. (#5189) * improve memtracker, add missed check & remove unnecessary thenError&tryCatch check (#5199) * [memtracker] check code run with memoery check on all works refine code all code memory checked fix lint refine code & fix build with gcc+sanitize * fix build break * fix lint * refine code * remove debug code * fix test fail build with debug * fix test fail build with debug * restore commented test * minor * minor * fix bug (#5214) * fix bug * fix bug Co-authored-by: Doodle <[email protected]> * handle rpc error task status (#5212) Co-authored-by: Sophie <[email protected]> * chore: community badges refined (#5202) * chore: community badges refined * Update README-CN.md * Update README-CN.md * Update README-CN.md remove sifou and zhihu as aligned with the team * update linkedin URL Co-authored-by: Yee <[email protected]> * Fix extend whtie space char. (#5213) * Fix extend whtie space char. * Format. Co-authored-by: Sophie <[email protected]> * Add lack tests of no role user. (#5196) Co-authored-by: Yee <[email protected]> * remove memtracker DLOG (#5224) * Add tck cases for DDL (#5220) * more TCK tests for variable pattern match clause (#5215) * cleanup * same src/dst for variable length pattern * variable pattern in where clause * variable scope tests in path pattern * More tests * More tests Co-authored-by: jimingquan <[email protected]> * Resumed the evaluation fo vertices in AttributeExpression (UTs included) (#5229) * add memtracker flags to conf (#5231) * add memtracker flags to conf * typo * refine * refine * add balance job type to filter when create backup (#5228) * add more job type to filter when create backup * log add job * add log before acquire snapshot lock Co-authored-by: Sophie <[email protected]> * Fix update sessions when leader change happens (#5225) * Fix udpate sessions when leader change happens * Handle errors on the graph side * Address comments * Address comments * fix match step range (#5216) * use smart pointer change raw pointer * fix error * fix test error * address comment Co-authored-by: Sophie <[email protected]> * Update response message when adding schema historically existed (#5227) * update the error code and message for checking history schemas * update tck * update comment * change to log error * fix ddl tck * increase wait time in schema.feature Co-authored-by: Sophie <[email protected]> * fix error code (#5233) Co-authored-by: Sophie <[email protected]> * print memory stats default to false (#5234) * print memory stats default to false * update conf Co-authored-by: Sophie <[email protected]> * fix bug of extract prop expr visitor (#5238) * forbid invalid prop expr used in cypher (#5242) * Fix mistake push down limit with skip. (#5241) Co-authored-by: Sophie <[email protected]> * fix delete fulltext index (#5239) * fix delete fulltext index * fix es delete error 1. remove get Rowreader if op is delete 2. delete es data when value is null Co-authored-by: Sophie <[email protected]> * Change the default value of session_reclaim_interval_secs to 60 seconds (#5246) Co-authored-by: Sophie <[email protected]> * Enhance attribute-accessing expression to ensure self-consistency (#5230) * Revert "Remove all UNKNOWN_PROP as a type of null. (#4907)" (#5149) This reverts commit aa62416. Co-authored-by: Sophie <[email protected]> * Enhance attribute-accessing expression to ensure self-consistency Fix tck Fix parser small delete Fix tck tck fmt fix ut fix ut Fix ut Fix tck Delete v.tag.prop check Fix tck Skip some tck cases related ngdata add test case Co-authored-by: Cheng Xuntao <[email protected]> Co-authored-by: Sophie <[email protected]> * fix ft index of fixed string (#5251) Co-authored-by: Sophie <[email protected]> * Add tck test (#5253) * add allpath test * add shortest path test case * add subgraph test case * add go test case * add go test case * Add more session tests (#5256) Co-authored-by: Sophie <[email protected]> * Revert "do not check term for leader info by default para" (#5266) This reverts commit 593bffc. * modify ft index default limit size (#5260) * modify ft index default limit size * fix test Co-authored-by: Doodle <[email protected]> * Test/yield (#5267) * Add some tests about yield. * Add more tests. Co-authored-by: Sophie <[email protected]> * Add another cert to test CA don't match. (#5247) Co-authored-by: Sophie <[email protected]> * fix baton miss reset in StorageJobExecutor (#5269) * Report errors on where clauses in optional match queries. (#5273) * [test case] Check DML cases (#5264) * Check DML cases * Add chinses char tests Add more tests Add mero delete edge tests * Revert cases * fix third party version in package.sh (#5281) The dump_syms tool path should be match with third party version. * Test/user (#5139) * Add some tests about user management. * Add tests about user roles. * Format. * Fix tck fixture name. * Fix step name. * Change step name. --------- Co-authored-by: Sophie <[email protected]> * fix https (#5283) * fix memtracker bugs during stress test on graphd and storaged (#5276) * fix memtracker bugs during stress test on graphd and storaged * fix lint * fix RocksEngine memory leak of raw pointer iter * add ENABLE_MEMORY_TRACKER build option & support adaptive limit for MemoryTracker * delete debug log * refine log * refine log * fix build * refine error log * print warning if memtracker is off * fix rocksdb leak by turn off memcheck * refine synamic-self-adaptive * fix cmake check * minor * minor * minor * minor * minor * refine double equel compare --------- Co-authored-by: jimingquan <[email protected]> Co-authored-by: jie.wang <[email protected]> Co-authored-by: Harris.Chu <[email protected]> Co-authored-by: Doodle <[email protected]> Co-authored-by: Yee <[email protected]> Co-authored-by: canon <[email protected]> Co-authored-by: Cheng Xuntao <[email protected]> Co-authored-by: hs.zhang <[email protected]> Co-authored-by: kyle.cao <[email protected]> Co-authored-by: Yichen Wang <[email protected]> Co-authored-by: liwenhui-soul <[email protected]> Co-authored-by: Alex Xing <[email protected]> Co-authored-by: codesigner <[email protected]> Co-authored-by: Wey Gu <[email protected]> Co-authored-by: shylock <[email protected]> Co-authored-by: pengwei.song <[email protected]> Co-authored-by: haowen <[email protected]> Co-authored-by: George <[email protected]>

fix memtracker bugs during stress test on graphd and storaged

09b934f

codesigner requested a review from critical27 as a code owner January 19, 2023 07:30

codesigner added the ready-for-testing PR: ready for the CI test label Jan 19, 2023

codesigner requested review from HarrisChu and kikimo January 19, 2023 07:31

critical27 previously approved these changes Jan 19, 2023

View reviewed changes

fix lint

9bb80c1

codesigner dismissed critical27’s stale review via 9bb80c1 January 19, 2023 09:01

fix RocksEngine memory leak of raw pointer iter

2287957

xtcyclist previously approved these changes Jan 19, 2023

View reviewed changes

add ENABLE_MEMORY_TRACKER build option & support adaptive limit for M…

9193087

…emoryTracker

codesigner dismissed xtcyclist’s stale review via 9193087 January 24, 2023 06:55

codesigner requested a review from a team as a code owner January 24, 2023 06:55

codesigner added 7 commits January 24, 2023 15:06

delete debug log

e6b3bfa

refine log

3eb48dd

refine log

d0eba2d

fix build

b997c02

refine error log

def33dd

print warning if memtracker is off

dd560d7

fix rocksdb leak by turn off memcheck

553dd4b

Sophie-Xie added the cherry-pick-v3.4 label Jan 28, 2023

codesigner added 8 commits January 29, 2023 10:18

Merge branch 'master' into memtracker-stable

884b207

refine synamic-self-adaptive

34ffe67

fix cmake check

c8e9946

minor

6f39ab0

minor

bafffa0

minor

a45f151

minor

8f5fe55

minor

665e000

critical27 previously approved these changes Jan 29, 2023

View reviewed changes

Merge branch 'master' into memtracker-stable

2ddefae

codesigner requested review from yixinglu, czpmango and jievince January 29, 2023 06:29

yixinglu reviewed Jan 29, 2023

View reviewed changes

codesigner added 2 commits January 29, 2023 15:27

Merge branch 'master' into memtracker-stable

9967f11

refine double equel compare

388d5b2

codesigner dismissed critical27’s stale review via 388d5b2 January 29, 2023 07:28

yixinglu approved these changes Jan 29, 2023

View reviewed changes

critical27 approved these changes Jan 29, 2023

View reviewed changes

Sophie-Xie merged commit cfb503e into vesoft-inc:master Jan 29, 2023

wey-gu mentioned this pull request Feb 4, 2023

Weekly Report 2023-02-03 vesoft-inc/nebula-community#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix memtracker bugs during stress test on graphd and storaged #5276

fix memtracker bugs during stress test on graphd and storaged #5276

codesigner commented Jan 19, 2023 •

edited

Loading

critical27 left a comment

yixinglu Jan 29, 2023

codesigner Jan 29, 2023 •

edited

Loading

yixinglu Jan 29, 2023

yixinglu Jan 29, 2023

yixinglu Jan 29, 2023

codesigner Jan 29, 2023

yixinglu Jan 29, 2023

codesigner Jan 29, 2023

yixinglu left a comment

fix memtracker bugs during stress test on graphd and storaged #5276

fix memtracker bugs during stress test on graphd and storaged #5276

Conversation

codesigner commented Jan 19, 2023 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

graphd

storaged

others

Tests

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

critical27 left a comment

Choose a reason for hiding this comment

yixinglu Jan 29, 2023

Choose a reason for hiding this comment

codesigner Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

yixinglu Jan 29, 2023

Choose a reason for hiding this comment

yixinglu Jan 29, 2023

Choose a reason for hiding this comment

yixinglu Jan 29, 2023

Choose a reason for hiding this comment

codesigner Jan 29, 2023

Choose a reason for hiding this comment

yixinglu Jan 29, 2023

Choose a reason for hiding this comment

codesigner Jan 29, 2023

Choose a reason for hiding this comment

yixinglu left a comment

Choose a reason for hiding this comment

codesigner commented Jan 19, 2023 •

edited

Loading

codesigner Jan 29, 2023 •

edited

Loading