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

Fix memory leak in Program::ReadFromFile() #36857

Closed
mkustermann opened this issue May 6, 2019 · 2 comments
Closed

Fix memory leak in Program::ReadFromFile() #36857

mkustermann opened this issue May 6, 2019 · 2 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@mkustermann
Copy link
Member

The code in runtime/vm/kernel_binary.cc:

Program* Program::ReadFromFile(const char* script_uri, const char** error /* = nullptr */) {
    ...
    uint8_t* kernel_buffer;
    intptr_t kernel_buffer_size;
    ...
    kernel_buffer = reinterpret_cast<uint8_t*>(malloc(kernel_buffer_size));
    memmove(kernel_buffer, data, kernel_buffer_size);
    ....
}

Seems to never get freed.

The usage of this buffer is in runtime/vm/isolate_reload.cc.

Notice that in the else branch we do attach a finalizer to an artificially created ExternalTypedData to ensure it's freed (at isolate shutdown time - we keep it around longer than necessary).

    // ReadKernelFromFile checks to see if the file at
    // root_script_url is a valid .dill file. If that's the case, a Program*
    // is returned. Otherwise, this is likely a source file that needs to be
    // compiled, so ReadKernelFromFile returns NULL.
    kernel_program.set(kernel::Program::ReadFromFile(root_script_url));
    ...
    if (kernel_program.get() != NULL) {
      num_received_libs_ = kernel_program.get()->library_count();
      bytes_received_libs_ = kernel_program.get()->kernel_data_size();
      p_num_received_classes = &num_received_classes_;
      p_num_received_procedures = &num_received_procedures_;
    } else {
      ...
      // The ownership of the kernel buffer goes now to the VM.
      const ExternalTypedData& typed_data = ExternalTypedData::Handle(
          Z,
          ExternalTypedData::New(kExternalTypedDataUint8ArrayCid, retval.kernel,
                                 retval.kernel_size, Heap::kOld));
      typed_data.AddFinalizer(
          retval.kernel,
          [](void* isolate_callback_data, Dart_WeakPersistentHandle handle,
             void* data) { free(data); },
          retval.kernel_size);

      // TODO(dartbug.com/33973): Change the heap objects to have a proper
      // retaining path to the kernel blob and ensure the finalizer will free it
      // once there are no longer references to it.
      // (The [ExternalTypedData] currently referenced by e.g. functions point
      // into the middle of c-allocated buffer and don't have a finalizer).
      I->RetainKernelBlob(typed_data);
      ...
    }

But if the hot-reload blob is loaded from the file, we don't do this.

@mkustermann mkustermann added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 6, 2019
@mkustermann
Copy link
Member Author

See also #33973 which tracks the fact that we don't want to keep the memory alive until isolate shutdown but rather want to keep it alive until it's not needed anymore.

@liamappelbe
Copy link
Contributor

The lifecycle of the kernel_data is complicated. It's passed to the Program, but then other things take references to the kernel_data (or views into the middle of it) that outlive the Program. It's a bit of a mess. Hopefully there's a single object somewhere (maybe the Library?) that has the same lifespan as the kernel_data that can conditionally take ownership of it (but only if it was constructed by an isolate_reload).

dart-bot pushed a commit that referenced this issue May 31, 2019
…r kernel buffers"

This reverts commit ab6aeaa.

Revert "[vm/compiler] Speed up the compiler part which deals with kernel reading up in DEBUG mode"

This reverts commit b316210.

Reason for revert: regression of snapshot sizes (DNO-599).

Original change's description:
> [vm/kernel] Use GC-tracked ExternalTypedData/TypedDataView for kernel buffers
>
> Until now we often leaked kernel buffers (e.g. hot reload buffers) because various
> objects were referencing ExternalTypedData objects pointing into the middle of
> c-allocated memory. This made it impossible for the GC to determine when the last
> reference is gone.
>
> This CL ensures that the actual buffers are *always* made available via
> ExternalTypedData and any inner pointers into it are created via TypedDataViews.
>
> The embedder guarantees to the free kernel buffers it has provided to:
>     - Dart_CreateIsolateFromKernel
>     - Dart_LoadScriptFromKernel
>     - Dart_LoadLibraryFromKernel
>     - Dart_SetDartLibrarySourcesKernel
> on isolate shutdown.
>
> All other kernel buffers will get a finalizer attached, which ensures the
> kernel buffers get freed by the GC once they are no longer referenced:
>     - Kernel blobs for expression evaluation
>     - Kernel blobs for Hot-Reload
>     - Kernel blobs for cc tests
>
> Fixes #33973
> Fixes #36857
> Issue #37030
>
> Change-Id: I1cc410c94c0f4b229413e793728a261afcb10aaf
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103130
> Reviewed-by: Ryan Macnak <rmacnak@google.com>
> Commit-Queue: Martin Kustermann <kustermann@google.com>

TBR=kustermann@google.com,rmacnak@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I49715d2400f4a5c8806b7d6a2912b7258f671a0a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/104343
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Auto-Submit: Alexander Markov <alexmarkov@google.com>
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, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

2 participants