Memory leak in C++ API with Core ML backend

[carsten-schultz-pix4d]

Describe the issue

Using the C++ API with the CoreML backend we observed memory leaks, in particular each call to Ort::Session::Run() leaked a significant amount of memory.

We were able to fix the particular leak with following patch, but the problem does not only affect this one call.

--- a/onnxruntime/core/providers/coreml/model/model.mm
+++ b/onnxruntime/core/providers/coreml/model/model.mm
@@ -333,7 +333,9 @@ Status Execution::Predict(const std::unordered_map<std::string, OnnxTensorData>&
   ORT_RETURN_IF_NOT(model_loaded, "Execution::Predict requires Execution::LoadModel");
 
   if (HAS_VALID_BASE_OS_VERSION) {
-    return [execution_ predict:inputs outputs:outputs];
+    @autoreleasepool {
+        return [execution_ predict:inputs outputs:outputs];
+    }
   }
 
   return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL, "Execution::LoadModel requires macos 10.15+ or ios 13+ ");

This code calls an Objective C method from Apple's Core ML API. These Objective C methods do not always fully release all of the temporary objects that they have created, they may only add them to the current autorelease pool, to be released when that pool is drained. However, the C++ code calling the Ort API has no idea of Objective C's autorelease pools, so none may exist or it may never be drained, and the objects are leaked.

Therefore the code in onnxruntime/core/providers/coreml which interfaces between C++ and Objective C would have to take care of creating and draining autorelease pools, but it currently doesn't.

One solution will be to sprinkle @autoreleaepool blocks around all calls to CoreML methods in the two Objective C++ files in coreml/model. I can certainly prepare a PR that does this, but maybe there is a more elegant solution. I do not normally write Objective C code, and someone here may feel ownership for the CoreML interface code.

(We are using 1.12.1, but I have not seen any relevant difference to the main branch.)

To reproduce

No example provided, sorry.

Urgency

No response

Platform

Mac

OS Version

12.6.2

ONNX Runtime Installation

Built from Source

ONNX Runtime Version or Commit ID

1.12.1

ONNX Runtime API

C++

Architecture

ARM64

Execution Provider

CoreML

Execution Provider Library Version

No response

[carsten-schultz-pix4d]

A bot tagged this for "mobile". We are dealing with macOS here, I specified the ARM architecture because the Macbook on which I tested happens to have an M1 processor. I do not know enough about iOS/iPadOS to say whether this is a problem on there, too. I do not know if pure C++ applications are even possible on those platforms, so the situation may be different.

[yuslepukhin]

You are welcome to submit a patch. Thanks for reporting it.

[skottmckay]

TBH we don't usually write Objective-C code either so we don't claim any expertise there.

Are there any potential issues with adding the @autoreleasepool blocks, in particular with the lifetime of objects that are returned from within the block?

I didn't notice any places in model.h that could be wrapped.

Would it just be things in the Execution class in model.mm that need to be wrapped if that's the class used to bridge to Objective-C (according to the comment anyway)?

[carsten-schultz-pix4d]

Thanks for coming back to this.

All expertise in Objective C memory handling that I can offer comes from one day of Internet search after locating the memory leak in our program. Doing a bit more reading right now I realise that that is certainly not enough. Also, looking again at model.mm I see that I had previously misidentified the border between onnxruntime and CoreML code.

The code change from my initial comment is used by us and has been tested to resolve the memory leak with any adverse effect. Of course it would be ideal to have extensive test coverage and run it with both ASAN and a leak checker, but the world is rarely ideal.

Looking at model.mm again I think that you are right that it would suffice to add the autorelease blocks in the Execution class. We have actually also added one in the block that contains [execution_ loadModel]. The statement in the constructor looks harmless, so that's probably it then. The relevant functions in Execution only return enums, so even not fully understanding all aspects of this it seems safe to me.

[edgchen1]

This issue about a memory leak from CoreML seems relevant.
https://developer.apple.com/forums/thread/692425

The suggested fix is to wrap the CoreML prediction call in an @autoreleasepool block.

[edgchen1]

@carsten-schultz-pix4d
Please see #14738. Tested it locally with a program that repeatedly calls Ort::Session::Run(). Not sure if it exactly matches your scenario, please feel free to verify the changes on your end.

[carsten-schultz-pix4d]

@edgchen1, thank you. I did not do further tests, but we use very similar changes.