gemini-code-assist[bot] commented on code in PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780540797
########## src/ffi/extra/copy.cc: ########## @@ -0,0 +1,287 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* + * \file src/ffi/extra/copy.cc + * + * \brief Reflection-based deep copy utilities. + */ +#include <tvm/ffi/any.h> +#include <tvm/ffi/container/array.h> +#include <tvm/ffi/container/map.h> +#include <tvm/ffi/container/shape.h> +#include <tvm/ffi/error.h> +#include <tvm/ffi/extra/copy.h> +#include <tvm/ffi/reflection/accessor.h> +#include <tvm/ffi/reflection/registry.h> + +#include <stack> +#include <unordered_map> +#include <unordered_set> +#include <vector> + +namespace tvm { +namespace ffi { + +/*! + * \brief Deep copier that recursively copies objects using shallow copy + field replacement. + * + * Algorithm: + * Phase 1: Walk the object graph using an explicit stack, collecting all reachable + * objects in topological order (post-order DFS). An unordered_set is used + * for deduplication. + * Phase 2: In topological order (leaves first), create shallow copies of each object + * and replace their fields with previously copied values. + * + * Primitive types, strings, bytes, and functions are immutable and returned as-is. + * Arrays and Maps are recursively copied (their elements are replaced with copies). + * Objects without enable_copy() are kept as shared references. + */ +class ObjectDeepCopier { + public: + static Any DeepCopy(const Any& root) { + ObjectDeepCopier copier; + // Phase 1: Collect all copyable objects in topological order + copier.CollectObjects(root); + // Phase 2: Copy objects in topological order (leaves first) + copier.CopyObjectsInOrder(); + // Phase 3: Resolve the root value with copies + return copier.Resolve(root); + } + + private: + struct DFSFrame { + const Object* obj; + bool children_pushed; + }; + + // The type attribute column for looking up per-type shallow copy functions + static reflection::TypeAttrColumn& ShallowCopyColumn() { + static reflection::TypeAttrColumn column("__ffi_shallow_copy__"); + return column; + } + + // Map from original object pointer to its shallow copy + std::unordered_map<const Object*, ObjectRef> copy_map_; + // Topological order of objects to copy (post-order, leaves first) + std::vector<const Object*> topo_order_; + // Set of visited object pointers for deduplication + std::unordered_set<const Object*> visited_; + + /*! + * \brief Phase 1: Walk the object graph, collecting copyable objects + * in topological order (post-order DFS). + */ + void CollectObjects(const Any& root) { WalkValue(root); } + + /*! + * \brief Recursively walk through Any values to find all objects. + */ + void WalkValue(const Any& value) { + switch (value.type_index()) { + case TypeIndex::kTVMFFIArray: { + Array<Any> arr = details::AnyUnsafe::CopyFromAnyViewAfterCheck<Array<Any>>(value); + for (const Any& elem : arr) { + WalkValue(elem); + } + break; + } + case TypeIndex::kTVMFFIMap: { + Map<Any, Any> m = details::AnyUnsafe::CopyFromAnyViewAfterCheck<Map<Any, Any>>(value); + for (const auto& [k, v] : m) { + WalkValue(k); + WalkValue(v); + } + break; + } + default: { + if (value.type_index() >= TypeIndex::kTVMFFIStaticObjectBegin) { + const Object* obj = details::AnyUnsafe::CopyFromAnyViewAfterCheck<const Object*>(value); + if (obj != nullptr && visited_.find(obj) == visited_.end()) { + CollectObjectDFS(obj); + } + } + break; + } + } + } Review Comment:  The traversal of containers (`Array`, `Map`) in `WalkValue`, `PushFieldChildren`, and `Resolve` is recursive. This could lead to a stack overflow for deeply nested containers (e.g., an `Array` of `Array`s). Since `CollectObjectDFS` already uses an iterative approach with an explicit stack to handle deep object graphs, I recommend applying a similar iterative pattern for container traversal here. This would make the deep copy mechanism fully robust against stack overflows for any object graph structure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
