-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Memory fixes. Resolves #10867, and resolves #14080 #14372
Changes from all commits
f81aec5
1205320
c27efd9
cb2542e
1dee791
5b8d88c
2f3d516
57e5f63
2f84c9d
4576cdc
0a1cbbb
5af91db
33c113c
232d33a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,8 +48,10 @@ class ResourceScope extends AutoCloseable { | |
| */ | ||
| override def close(): Unit = { | ||
| ResourceScope.removeFromThreadLocal(this) | ||
| resourceQ.foreach(resource => if (resource != null) resource.dispose(false) ) | ||
| resourceQ.clear() | ||
| if (!ResourceScope.threadLocalScopes.get().contains(this)) { | ||
| resourceQ.foreach(resource => if (resource != null) resource.dispose(false)) | ||
| resourceQ.clear() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -145,7 +147,7 @@ object ResourceScope { | |
| null.asInstanceOf[A] // we'll throw in finally | ||
| } finally { | ||
| var toThrow: Throwable = retThrowable | ||
| if (retThrowable eq null) curScope.close() | ||
| if (retThrowable eq null) curScope.close | ||
| else { | ||
| try { | ||
| curScope.close | ||
|
|
@@ -160,6 +162,17 @@ object ResourceScope { | |
| } | ||
| } | ||
|
|
||
| private[mxnet] def usingIfScopeExists[A](scope: Option[ResourceScope])(body: => A): A = { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need this new method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah so originally I wanted to use the .using method directly but ran into some problems. There are times when we need to add new native resources to the same scope of the parent but there's no guarantee that the parent is actually in an existing scope. When this happens it leads to a few issues. First, the parent is in None scope which we cannot pass to ResourceScope.using. Second, we still want to execute the body and allocate all the new resources but don't want to want to make a new ResourceScope because all those new resources will disappear with the new scope. Alternatives that I thought of were: 1.) to have the caller check whether or not it was in a scope and handle it appropriately. This is ugly and puts the onus on the callers in multiple places. 2.) Changing the using method to work with a None scope. I opted not to do this because it complicates that method and I believe would require changing the method parameters which I didn't want to do. 3.) Changing the default scope to be something other than None. This is probably a reasonable solution. Maybe we have some kind of base scope or something similar. That's likely to be a fairly significant change in both the design and behavior of this class. |
||
| if (scope == None) { | ||
| body | ||
| } else { | ||
| ResourceScope.addToThreadLocal(scope.get) | ||
andrewfayres marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ResourceScope.using(scope.get){ | ||
| body | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // thread local Scopes | ||
| private[mxnet] val threadLocalScopes = new ThreadLocal[ArrayBuffer[ResourceScope]] { | ||
| override def initialValue(): ArrayBuffer[ResourceScope] = | ||
|
|
@@ -179,7 +192,7 @@ object ResourceScope { | |
| * @param r ResourceScope to remove | ||
| */ | ||
| private[mxnet] def removeFromThreadLocal(r: ResourceScope): Unit = { | ||
| threadLocalScopes.get() -= r | ||
| threadLocalScopes.get().remove(threadLocalScopes.get().lastIndexOf(r)) | ||
andrewfayres marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.