Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad memory-retention with non-capturing closures #18886

Closed
floitschG opened this issue May 19, 2014 · 6 comments
Closed

Bad memory-retention with non-capturing closures #18886

floitschG opened this issue May 19, 2014 · 6 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@floitschG
Copy link
Contributor

The VM sometimes keep unrelated memory alive when it constructs non-capturing closures.
When the parser detects that a closure doesn't have any free variables it avoids the context creation, but accidentally reuses the one that was passed in, which could hold onto random objects.
Dart2js, for example, held onto the main-compilation object (which basically meant almost everything) and ran out of memory.

Our batch-mode (halving the compilation time on the build-bot) is blocked by this bug, so I'm assigning priority high to it.
We could work around it, though. If fixing it turns out to be difficult let us know and we will implement the work-around.

With the following patch dart2js runs without oom:

diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc
index 66018e5..e54cf32 100644

--- a/runtime/vm/parser.cc
+++ b/runtime/vm/parser.cc
@@ -185,7 +185,7 @­@ void ParsedFunction::AllocateVariables() {
   // had the effect of unintentionally retaining parent contexts which
   // would never be accessed. By breaking the context chain at this
   // point, we allow these outer contexts to be collected.

  • if (found_captured_variables) {
  • if (true || found_captured_variables) {
         const ContextScope& context_scope =
             ContextScope::Handle(function().context_scope());
         if (context_scope.IsNull() || (context_scope.num_variables() == 0)) {

diff --git a/runtime/vm/flow_graph_type_propagator.cc b/runtime/vm/flow_graph_type_propagator.cc
index d2c72bd..84356cf 100644

--- a/runtime/vm/flow_graph_type_propagator.cc
+++ b/runtime/vm/flow_graph_type_propagator.cc
@@ -552,6 +552,9 @­@ const AbstractType* CompileType::ToAbstractType() {
     if (cid_ == kFunctionCid) {
       type_ = &Type::ZoneHandle(Type::DynamicType());
       return type_;

  • } else if (cid_ == kContextCid) {
  •  type_ = &Type::ZoneHandle(Type::DynamicType());
    
  •  return type_;
    
         }
     
@floitschG
Copy link
Contributor Author

Thanks to Slava for helping me debug.

@kasperl
Copy link

kasperl commented Jul 10, 2014

Added this to the 1.6 milestone.

@iposva-google
Copy link
Contributor

I am having a hard time figuring out what pattern you are referring to that keeps objects alive which should not be alive any longer. Can you please provide an example?


Removed this from the 1.6 milestone.
Removed Priority-High label.
Added Priority-Medium label.

@mraleph
Copy link
Member

mraleph commented Jul 31, 2014

Here is a repro:

foo() {
  return () { };
}

bar(a, b) {
  f() => [a, b];
  return foo();
}

main() {
  var closure = null;
  do {
    closure = bar(closure, new List(1024 * 1024));
  } while (true);
}

will crash with OOM because the context of f allocated within bar is actually leaking and becoming the context of empty non-capturing closure allocated inside foo. If we declare foo like this instead:

foo() {
  var x;
  return () { x; };
}

then this does not happen.

@fsc8000
Copy link
Contributor

fsc8000 commented Oct 30, 2014

My upcoming change of how the VM handles closure contexts will fix this issue.


Set owner to @fsc8000.
Added Accepted label.

@fsc8000
Copy link
Contributor

fsc8000 commented Oct 31, 2014

Fixed with r41422 and r41433.


Added Fixed label.

@floitschG floitschG added Type-Defect area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. labels Oct 31, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

5 participants