Made it so you can specify the old gen heap size. #15259

gaaclarke · 2020-01-07T21:33:32Z

related issue: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=40877#c19

cbracken · 2020-01-07T21:42:08Z

runtime/dart_vm.cc

@@ -111,6 +111,15 @@ static const char* kDartTraceStreamsArgs[] = {
    "--timeline_streams=Compiler,Dart,Debugger,Embedder,GC,Isolate,VM,API",
 };

+static std::string DartOldGenHeapSizeArgs(uint64_t heap_size) {
+  static const int32_t buffer_size = 100;


Use constexpr (though I believe the compiler will promote this to constexpr anyway)

cbracken · 2020-01-07T21:52:03Z

We'll want to expose this to embedders, but feel free to do that in a separate PR(s). The only requests we've had so far have been for Fuchsia (and likely a customer using the embedder API).

cbracken · 2020-01-07T21:53:26Z

runtime/dart_vm.cc

+  constexpr int32_t buffer_size = 100;
+  const char* flag = "--old_gen_heap_size=";
+  char buffer[buffer_size];
+  int result = snprintf(buffer, buffer_size, "%s%lld", flag, heap_size);


../../flutter/runtime/dart_vm.cc:118:62: error: format specifies type 'long long' but the argument has type 'uint64_t' (aka 'unsigned long') [-Werror,-Wformat] int result = snprintf(buffer, buffer_size, "%s%lld", flag, heap_size); ~~~~ ^~~~~~~~~ %lu

jason-simmons · 2020-01-07T21:56:40Z

runtime/dart_vm.cc

+  constexpr int32_t buffer_size = 100;
+  const char* flag = "--old_gen_heap_size=";
+  char buffer[buffer_size];
+  int result = snprintf(buffer, buffer_size, "%s%" PRId64, flag, heap_size);


Use std::ostringstream here

I used snprintf because my concern was binary size. Is there a reason to use string streams?

Not really an issue here but snprintf is faster because it works on a fixed size buffer and doesn't have to juggle memory allocations: https://stackoverflow.com/a/445348

IMHO the code will be simpler with reduced risk of buffer overflows. We already use iostreams in many places throughout the engine, and any performance difference will be negligible given that this is a one time operation during startup of the runtime.

You can't get an overflow with snprintf and a buffer with the correct size, but switched to sstream to match other code.

gaaclarke · 2020-01-07T22:43:44Z

We'll want to expose this to embedders, but feel free to do that in a separate PR(s). The only requests we've had so far have been for Fuchsia (and likely a customer using the embedder API).

Done.

gaaclarke · 2020-01-08T18:20:42Z

Landing with Linux Fuchsia purple: https://ci.chromium.org/p/flutter/builders/try/Linux%20Fuchsia/1525

This is an infra problem.

…gine#15259)

rmacnak-google · 2020-01-08T20:43:00Z

runtime/dart_vm_unittests.cc

+  settings.old_gen_heap_size = 1024;
+  auto vm = DartVMRef::Create(settings);
+  // There is no way to introspect on the heap size so we just assert the vm was
+  // created.


You might try to allocate an array larger than the max size and check it produces an OutOfMemory exception.

cbracken

LGTM

flutter/engine@f001ea2...46adf73 git log f001ea2..46adf73 --first-parent --oneline 2020-01-08 30870216+gaaclarke@users.noreply.github.com Made it so you can specify the old gen heap size. (flutter/engine#15259) 2020-01-08 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from gL-XG... to rTJJV... (flutter/engine#15308) 2020-01-08 xster@google.com Add a deprecation javadoc note to the old FlutterActivity (flutter/engine#15156) 2020-01-08 31859944+LongCatIsLooong@users.noreply.github.com Bump simulator version in IosUnitTests & scenario app in preparation for luci xcode 11 migration (flutter/engine#15154) 2020-01-08 skia-flutter-autoroll@skia.org Roll src/third_party/skia 91e0d7526944..f811fc331a14 (32 commits) (flutter/engine#15306) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC garyq@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

…gine#15259)

)" This reverts commit 8d790a1.

Made it so you can specify the old gen heap size.

3706fb6

auto-assign bot requested a review from flar January 7, 2020 21:33

googlebot added cla: yes labels Jan 7, 2020

gaaclarke added Work in progress (WIP) Not ready (yet) for review! and removed Work in progress (WIP) Not ready (yet) for review! labels Jan 7, 2020

gaaclarke requested review from cbracken and jason-simmons January 7, 2020 21:38

cbracken reviewed Jan 7, 2020

View reviewed changes

ran formatter

a1420cd

switched to constexpr

b9f59ab

cbracken reviewed Jan 7, 2020

View reviewed changes

fixed snprintf variable

49d0f0d

jason-simmons reviewed Jan 7, 2020

View reviewed changes

exposed the flag to embedder

c75eb3b

gaaclarke requested review from cbracken and jason-simmons January 7, 2020 22:54

switched to stringstreams

f637990

jason-simmons approved these changes Jan 7, 2020

View reviewed changes

gaaclarke merged commit 46adf73 into flutter:master Jan 8, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 8, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

57dc622

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 8, 2020

Roll engine f001ea29f1b9..46adf73d5128 (5 commits) flutter/flutter#48437

Merged

rmacnak-google reviewed Jan 8, 2020

View reviewed changes

cbracken reviewed Jan 8, 2020

View reviewed changes

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

2cf62ec

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48587

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

cb9c427

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48588

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

1ec62b8

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48590

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

39942a7

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48592

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

13e6057

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48593

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

545c613

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48594

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

a8c0438

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48595

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

a9edbf2

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 10, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48596

Closed

GaryQian mentioned this pull request Jan 10, 2020

Manually Roll engine to f001ea2...7ef88f8 (34 commits) to resolve goldens flutter/flutter#48598

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 11, 2020

46adf73 Made it so you can specify the old gen heap size. (flutter/en…

be656d4

…gine#15259)

engine-flutter-autoroll mentioned this pull request Jan 11, 2020

Roll engine f001ea29f1b9..7ef88f85d572 (34 commits) flutter/flutter#48615

Closed

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Made it so you can specify the old gen heap size. (flutter#15259)

8d790a1

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Made it so you can specify the old gen heap size. (flutter#15259

adfcdbb

)" This reverts commit 8d790a1.

filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020

Made it so you can specify the old gen heap size. (flutter#15259)

f209e85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made it so you can specify the old gen heap size. #15259

Made it so you can specify the old gen heap size. #15259

gaaclarke commented Jan 7, 2020 •

edited

cbracken Jan 7, 2020

cbracken commented Jan 7, 2020

cbracken Jan 7, 2020

gaaclarke Jan 7, 2020

jason-simmons Jan 7, 2020

gaaclarke Jan 7, 2020

gaaclarke Jan 7, 2020

jason-simmons Jan 7, 2020

gaaclarke Jan 7, 2020

gaaclarke commented Jan 7, 2020

gaaclarke commented Jan 8, 2020

rmacnak-google Jan 8, 2020

cbracken left a comment

Made it so you can specify the old gen heap size. #15259

Made it so you can specify the old gen heap size. #15259

Conversation

gaaclarke commented Jan 7, 2020 • edited

Choose a reason for hiding this comment

cbracken commented Jan 7, 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

gaaclarke commented Jan 7, 2020

gaaclarke commented Jan 8, 2020

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

gaaclarke commented Jan 7, 2020 •

edited