[v8_engine]: Add settings for max_old_gen_size #3167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenPope
reviewed
Dec 8, 2021
| @@ -44,6 +45,7 @@ class script_exception final : public std::exception { | |||
| class script { | |||
| // Timeout for run script in first time for initialization | |||
| static constexpr std::chrono::milliseconds _first_run_timeout_ms{500}; | |||
| static constexpr uint64_t _max_old_gen_size{10_MiB}; | |||
There was a problem hiding this comment.
How is this value chosen? Should it be configurable?
There was a problem hiding this comment.
With this value test works.
I want to return to this settings in data_policy testing
There was a problem hiding this comment.
I think Ben is asking why 10MiB? Was this the value older versions of v8 were using before it was exposed?
There was a problem hiding this comment.
My ans is It is the most smallest value when test works(can create local context for v8).
In old v8 versions it was self calculated params, but in new v8 versions we need to use ConfigureDefaultsFromHeapSize together with set_max_old_generation_size_in_bytes. Need to understand limits before data-policy release redpanda-data#3166
VadimPlh
force-pushed
the
prepared-v8-for-llvm13
branch
from
ca62737
to
4705e45
Compare
BenPope
approved these changes
Dec 8, 2021
dotnwat
approved these changes
Dec 8, 2021
| @@ -44,6 +45,7 @@ class script_exception final : public std::exception { | |||
| class script { | |||
| // Timeout for run script in first time for initialization | |||
| static constexpr std::chrono::milliseconds _first_run_timeout_ms{500}; | |||
| static constexpr uint64_t _max_old_gen_size{10_MiB}; | |||
There was a problem hiding this comment.
stylistically we generally don't prefix _ on configuration options like this even though they are technically private members.
yokofly
added a commit
to timeplus-io/proton
that referenced
this pull request
May 14, 2024
as the redpanda-data/redpanda#3167 we need set both old_gen and default_heap. also close #574 (is this issue description too boring?)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In old v8 versions it was self calculated params, but in new v8 versions we need to use
ConfigureDefaultsFromHeapSizetogether withset_max_old_generation_size_in_bytes.Need to understand limits before data-policy release
#3166
Without
set_max_old_generation_size_in_bytestests for v8_engine fail with new v8 version.Fix for llvm, seastar, v8 update https://github.com/vectorizedio/vtools/pull/316