From 6c0899d2136d83fc7edeb68b08ffc538efa30c5b Mon Sep 17 00:00:00 2001 From: Ping Yu <4018+pyu10055@users.noreply.github.com> Date: Thu, 6 Aug 2020 10:50:52 -0700 Subject: [PATCH 1/3] ensure clone is done only for not kept tensors --- .../operations/executors/control_executor.ts | 49 +++++++++++++------ .../operations/executors/graph_executor.ts | 18 +++---- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/tfjs-converter/src/operations/executors/control_executor.ts b/tfjs-converter/src/operations/executors/control_executor.ts index 7effe6f6547..2ee725f7409 100644 --- a/tfjs-converter/src/operations/executors/control_executor.ts +++ b/tfjs-converter/src/operations/executors/control_executor.ts @@ -106,44 +106,63 @@ export const executeOp: InternalOpAsyncExecutor = async( return result; } case 'LoopCond': { - return [ - (getParamValue('pred', node, tensorMap, context) as tfc.Tensor).clone() - ]; + let pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; + if (!pred.kept) { + pred = pred.clone(); + } + + return [pred]; } case 'Switch': { const pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; - const data = - getParamValue('data', node, tensorMap, context) as tfc.Tensor; + let data = getParamValue('data', node, tensorMap, context) as tfc.Tensor; + if (!data.kept) { + data = data.clone(); + } // Outputs nodes :0 => false, :1 => true - return (await pred.data())[0] ? [undefined, data.clone()] : - [data.clone(), undefined]; + return (await pred.data())[0] ? [undefined, data] : [data, undefined]; } case 'Merge': { const inputName = node.inputNames.find( name => getTensor(name, tensorMap, context) !== undefined); - return inputName ? [getTensor(inputName, tensorMap, context).clone()] : - undefined; + if (inputName) { + let data = getTensor(inputName, tensorMap, context); + if (!data.kept) { + data = data.clone(); + } + return [data]; + } + return undefined; } case 'Enter': { const frameId = getParamValue('frameName', node, tensorMap, context) as string; - const data = + let data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.enterFrame(frameId); - return [data.clone()]; + if (!data.kept) { + data = data.clone(); + } + return [data]; } case 'Exit': { - const tensor = + let data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.exitFrame(); - return [tensor.clone()]; + if (!data.kept) { + data = data.clone(); + } + return [data]; } case 'NextIteration': { - const input = + let data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.nextIteration(); - return [input.clone()]; + if (!data.kept) { + data = data.clone(); + } + return [data]; } case 'TensorArrayV3': { const size = getParamValue('size', node, tensorMap, context) as number; diff --git a/tfjs-converter/src/operations/executors/graph_executor.ts b/tfjs-converter/src/operations/executors/graph_executor.ts index c788577f89f..bf608057078 100644 --- a/tfjs-converter/src/operations/executors/graph_executor.ts +++ b/tfjs-converter/src/operations/executors/graph_executor.ts @@ -24,9 +24,9 @@ import {InternalOpExecutor, Node} from '../types'; import {getParamValue, getTensor} from './utils'; export const executeOp: InternalOpExecutor = (node: Node, - tensorMap: NamedTensorsMap, - context: ExecutionContext): - tfc.Tensor[] => { + tensorMap: NamedTensorsMap, + context: ExecutionContext): + tfc.Tensor[] => { switch (node.op) { case 'Const': { return tensorMap[node.name]; @@ -39,17 +39,17 @@ export const executeOp: InternalOpExecutor = (node: Node, return [getTensor(node.name, tensorMap, context)]; case 'Identity': case 'StopGradient': - case 'FakeQuantWithMinMaxVars': // This op is currently ignored. - return [ - (getParamValue('x', node, tensorMap, context) as tfc.Tensor).clone() - ]; + case 'FakeQuantWithMinMaxVars': { // This op is currently ignored. + const data = getParamValue('x', node, tensorMap, context) as tfc.Tensor; + return [data.kept ? data : data.clone()]; + } case 'IdentityN': return (getParamValue('x', node, tensorMap, context) as tfc.Tensor[]) - .map((t: tfc.Tensor) => t.clone()); + .map((t: tfc.Tensor) => t.kept ? t : t.clone()); case 'Snapshot': const snapshot = (getParamValue('x', node, tensorMap, context) as tfc.Tensor); - return [snapshot.clone()]; + return [snapshot.kept ? snapshot : snapshot.clone()]; case 'Shape': return [tfc.tensor1d( (getParamValue('x', node, tensorMap, context) as tfc.Tensor).shape, From 494b28aae361be161e9a75984f6091058f4a0faa Mon Sep 17 00:00:00 2001 From: Ping Yu <4018+pyu10055@users.noreply.github.com> Date: Thu, 6 Aug 2020 11:16:24 -0700 Subject: [PATCH 2/3] use tfc.clone and added comments --- .../operations/executors/control_executor.ts | 49 +++++++++---------- .../operations/executors/graph_executor.ts | 12 +++-- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/tfjs-converter/src/operations/executors/control_executor.ts b/tfjs-converter/src/operations/executors/control_executor.ts index 2ee725f7409..dc06b4cbd8f 100644 --- a/tfjs-converter/src/operations/executors/control_executor.ts +++ b/tfjs-converter/src/operations/executors/control_executor.ts @@ -106,19 +106,18 @@ export const executeOp: InternalOpAsyncExecutor = async( return result; } case 'LoopCond': { - let pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; - if (!pred.kept) { - pred = pred.clone(); - } - - return [pred]; + const pred = + getParamValue('pred', node, tensorMap, context) as tfc.Tensor; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [pred.kept ? pred : tfc.clone(pred)]; } case 'Switch': { const pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; let data = getParamValue('data', node, tensorMap, context) as tfc.Tensor; if (!data.kept) { - data = data.clone(); + data = tfc.clone(data); } // Outputs nodes :0 => false, :1 => true return (await pred.data())[0] ? [undefined, data] : [data, undefined]; @@ -127,42 +126,38 @@ export const executeOp: InternalOpAsyncExecutor = async( const inputName = node.inputNames.find( name => getTensor(name, tensorMap, context) !== undefined); if (inputName) { - let data = getTensor(inputName, tensorMap, context); - if (!data.kept) { - data = data.clone(); - } - return [data]; + const data = getTensor(inputName, tensorMap, context); + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [data.kept ? data : tfc.clone(data)]; } return undefined; } case 'Enter': { const frameId = getParamValue('frameName', node, tensorMap, context) as string; - let data = + const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.enterFrame(frameId); - if (!data.kept) { - data = data.clone(); - } - return [data]; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [data.kept ? data : tfc.clone(data)]; } case 'Exit': { - let data = + const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.exitFrame(); - if (!data.kept) { - data = data.clone(); - } - return [data]; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [data.kept ? data : tfc.clone(data)]; } case 'NextIteration': { - let data = + const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.nextIteration(); - if (!data.kept) { - data = data.clone(); - } - return [data]; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [data.kept ? data : tfc.clone(data)]; } case 'TensorArrayV3': { const size = getParamValue('size', node, tensorMap, context) as number; diff --git a/tfjs-converter/src/operations/executors/graph_executor.ts b/tfjs-converter/src/operations/executors/graph_executor.ts index bf608057078..fc7c25fa2d0 100644 --- a/tfjs-converter/src/operations/executors/graph_executor.ts +++ b/tfjs-converter/src/operations/executors/graph_executor.ts @@ -41,15 +41,21 @@ export const executeOp: InternalOpExecutor = (node: Node, case 'StopGradient': case 'FakeQuantWithMinMaxVars': { // This op is currently ignored. const data = getParamValue('x', node, tensorMap, context) as tfc.Tensor; - return [data.kept ? data : data.clone()]; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [data.kept ? data : tfc.clone(data)]; } case 'IdentityN': + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. return (getParamValue('x', node, tensorMap, context) as tfc.Tensor[]) - .map((t: tfc.Tensor) => t.kept ? t : t.clone()); + .map((t: tfc.Tensor) => t.kept ? t : tfc.clone(t)); case 'Snapshot': const snapshot = (getParamValue('x', node, tensorMap, context) as tfc.Tensor); - return [snapshot.kept ? snapshot : snapshot.clone()]; + // Reuse the tensor if has marked as keep, otherwise clone the tensor to + // avoid disposal. + return [snapshot.kept ? snapshot : tfc.clone(snapshot)]; case 'Shape': return [tfc.tensor1d( (getParamValue('x', node, tensorMap, context) as tfc.Tensor).shape, From 2c2806a09e63a1c2615eabf54dc3ed3dd5abed72 Mon Sep 17 00:00:00 2001 From: Ping Yu <4018+pyu10055@users.noreply.github.com> Date: Thu, 6 Aug 2020 15:11:12 -0700 Subject: [PATCH 3/3] refactor the clone to a util method, and add comments --- .../operations/executors/control_executor.ts | 24 ++++++------------- .../operations/executors/graph_executor.ts | 14 ++++------- .../src/operations/executors/utils.ts | 13 ++++++++++ 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/tfjs-converter/src/operations/executors/control_executor.ts b/tfjs-converter/src/operations/executors/control_executor.ts index dc06b4cbd8f..5decb09c4c2 100644 --- a/tfjs-converter/src/operations/executors/control_executor.ts +++ b/tfjs-converter/src/operations/executors/control_executor.ts @@ -24,7 +24,7 @@ import {TensorArray} from '../../executor/tensor_array'; import {fromTensor, reserve, scatter, split} from '../../executor/tensor_list'; import {InternalOpAsyncExecutor, Node} from '../types'; -import {getParamValue, getTensor} from './utils'; +import {cloneTensor, getParamValue, getTensor} from './utils'; export const executeOp: InternalOpAsyncExecutor = async( node: Node, tensorMap: NamedTensorsMap, @@ -108,16 +108,14 @@ export const executeOp: InternalOpAsyncExecutor = async( case 'LoopCond': { const pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [pred.kept ? pred : tfc.clone(pred)]; + return [cloneTensor(pred)]; } case 'Switch': { const pred = getParamValue('pred', node, tensorMap, context) as tfc.Tensor; let data = getParamValue('data', node, tensorMap, context) as tfc.Tensor; if (!data.kept) { - data = tfc.clone(data); + data = cloneTensor(data); } // Outputs nodes :0 => false, :1 => true return (await pred.data())[0] ? [undefined, data] : [data, undefined]; @@ -127,9 +125,7 @@ export const executeOp: InternalOpAsyncExecutor = async( name => getTensor(name, tensorMap, context) !== undefined); if (inputName) { const data = getTensor(inputName, tensorMap, context); - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [data.kept ? data : tfc.clone(data)]; + return [cloneTensor(data)]; } return undefined; } @@ -139,25 +135,19 @@ export const executeOp: InternalOpAsyncExecutor = async( const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.enterFrame(frameId); - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [data.kept ? data : tfc.clone(data)]; + return [cloneTensor(data)]; } case 'Exit': { const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.exitFrame(); - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [data.kept ? data : tfc.clone(data)]; + return [cloneTensor(data)]; } case 'NextIteration': { const data = getParamValue('tensor', node, tensorMap, context) as tfc.Tensor; context.nextIteration(); - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [data.kept ? data : tfc.clone(data)]; + return [cloneTensor(data)]; } case 'TensorArrayV3': { const size = getParamValue('size', node, tensorMap, context) as number; diff --git a/tfjs-converter/src/operations/executors/graph_executor.ts b/tfjs-converter/src/operations/executors/graph_executor.ts index fc7c25fa2d0..fc7b720fc60 100644 --- a/tfjs-converter/src/operations/executors/graph_executor.ts +++ b/tfjs-converter/src/operations/executors/graph_executor.ts @@ -21,7 +21,7 @@ import {NamedTensorsMap} from '../../data/types'; import {ExecutionContext} from '../../executor/execution_context'; import {InternalOpExecutor, Node} from '../types'; -import {getParamValue, getTensor} from './utils'; +import {cloneTensor, getParamValue, getTensor} from './utils'; export const executeOp: InternalOpExecutor = (node: Node, tensorMap: NamedTensorsMap, @@ -41,21 +41,15 @@ export const executeOp: InternalOpExecutor = (node: Node, case 'StopGradient': case 'FakeQuantWithMinMaxVars': { // This op is currently ignored. const data = getParamValue('x', node, tensorMap, context) as tfc.Tensor; - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [data.kept ? data : tfc.clone(data)]; + return [cloneTensor(data)]; } case 'IdentityN': - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. return (getParamValue('x', node, tensorMap, context) as tfc.Tensor[]) - .map((t: tfc.Tensor) => t.kept ? t : tfc.clone(t)); + .map((t: tfc.Tensor) => cloneTensor(t)); case 'Snapshot': const snapshot = (getParamValue('x', node, tensorMap, context) as tfc.Tensor); - // Reuse the tensor if has marked as keep, otherwise clone the tensor to - // avoid disposal. - return [snapshot.kept ? snapshot : tfc.clone(snapshot)]; + return [cloneTensor(snapshot)]; case 'Shape': return [tfc.tensor1d( (getParamValue('x', node, tensorMap, context) as tfc.Tensor).shape, diff --git a/tfjs-converter/src/operations/executors/utils.ts b/tfjs-converter/src/operations/executors/utils.ts index 55af5eafd52..97fae50f1b8 100644 --- a/tfjs-converter/src/operations/executors/utils.ts +++ b/tfjs-converter/src/operations/executors/utils.ts @@ -136,3 +136,16 @@ export function getPadding( } return pad; } + +/** + * Reuse the tensor if it is marked as keep, otherwise clone the tensor to + * avoid disposal. This is important for TensorArray and TensorList ops, since + * internally they use a tensor as the id for TensorArray and TensorList, and + * to simplify lookup, they also use Tensor.id as the key to the internal map. + * These id tensors have been marked as kept in the backend, we need avoid clone + * them in order to create new Tensor.id. + * @param tensor + */ +export function cloneTensor(tensor: tfc.Tensor): tfc.Tensor { + return tensor.kept ? tensor : tfc.clone(tensor); +}