Skip to content

Conversation

@abyss7
Copy link
Collaborator

@abyss7 abyss7 commented Dec 7, 2024

Changelog category

  • Improvement

@abyss7 abyss7 requested a review from kungasc December 7, 2024 13:19
@github-actions
Copy link

github-actions bot commented Dec 7, 2024

2024-12-07 13:21:01 UTC Pre-commit check linux-x86_64-release-asan for 1d3750b has started.
2024-12-07 13:21:10 UTC Artifacts will be uploaded here
2024-12-07 13:24:00 UTC ya make is running...
🟡 2024-12-07 14:47:12 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
12911 12835 0 22 8 46

🟢 2024-12-07 14:48:17 UTC Build successful.
🟢 2024-12-07 14:48:47 UTC ydbd size 4.9 GiB changed* by -1.0 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash main: 817dc99 merge: 1d3750b diff diff %
ydbd size 5 305 900 056 Bytes 5 305 899 000 Bytes -1.0 KiB -0.000%
ydbd stripped size 1 365 606 224 Bytes 1 365 605 968 Bytes -256 Bytes -0.000%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@github-actions
Copy link

github-actions bot commented Dec 7, 2024

2024-12-07 13:21:51 UTC Pre-commit check linux-x86_64-relwithdebinfo for 1d3750b has started.
2024-12-07 13:22:03 UTC Artifacts will be uploaded here
2024-12-07 13:24:55 UTC ya make is running...
🟡 2024-12-07 14:11:18 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
20428 18920 0 1 1397 110

2024-12-07 14:12:57 UTC ya make is running... (failed tests rerun, try 2)
🟢 2024-12-07 14:32:04 UTC Tests successful.

Test history | Ya make output | Test bloat | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
639 (only retried tests) 532 0 0 1 106

🟢 2024-12-07 14:32:13 UTC Build successful.
🟢 2024-12-07 14:32:30 UTC ydbd size 2.5 GiB changed* by -96 Bytes, which is <= 0 Bytes vs main: OK

ydbd size dash main: 817dc99 merge: 1d3750b diff diff %
ydbd size 2 699 385 480 Bytes 2 699 385 384 Bytes -96 Bytes -0.000%
ydbd stripped size 483 518 032 Bytes 483 518 032 Bytes 0 Bytes 0.000%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

NKikimrConsole::TConfigItem::MemoryControllerConfigItem}));

// When profiling memory it's convenient to set initial tcmalloc soft limit
#ifdef PROFILE_MEMORY_ALLOCATIONS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Не понятно почему этот код не может работать без PROFILE_MEMORY_ALLOCATIONS
  2. Настройки у нас горячие, поэтому и выставлять лимит надо в HandleWakeup
  3. Не понятно почему выставляем именно SoftLimit. Что будет когда нам потребуется больше памяти?

Copy link
Contributor

@kungasc kungasc Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я бы лучше добавил отдельную новую настройку в TMemoryControllerConfig, например AllocatorLimit

Но надо тогда написать как будет всё работать при её выставлении (и желательно и в доку)

Copy link
Collaborator Author

@abyss7 abyss7 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Без этого флага он (лимит) сейчас примерно бесполезен для kqp - и при его достижении выполняется не то чтобы сильно отлаженный код внутри самого tcmalloc. Поэтому он точно должен быть как минимум в режиме отладки - когда после достижения лимита нестрашно и упасть.
  2. Сценарий изменения настройки SoftLimit именно для tcmalloc другой: она сама горячая и мне удобнее при отладке менять её налету отдельно - без постоянной интерференции со стороны HandleWakeup, который не реагирует на фактическое изменение HardLimit, а просто всегда выставляет известные ему значения.
  3. Потому что логика колбека с печатью кучи завязана именно на SoftLimit. Ну и мы вроде корректно транслируем один SoftLimit в другой - либо я не понял вопрос.

@kungasc
Copy link
Contributor

kungasc commented Dec 9, 2024

Fine as a temporary solution before #12406

@abyss7 abyss7 merged commit a9cc770 into ydb-platform:main Dec 9, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants