Mark thread-safe methods of DataLoader as const. (#4756)

shoumikhin · facebook-github-bot · commit ce5793681983 · 2024-08-16T14:37:29.000-07:00
Summary: Pull Request resolved: #4756 Every DataLoader must not modify its internal state after initialization so that it can be shared and used by multiple threads simultaneously. Reviewed By: dbort Differential Revision: D61406229
diff --git a/backends/apple/coreml/runtime/test/CoreMLBackendDelegateTests.mm b/backends/apple/coreml/runtime/test/CoreMLBackendDelegateTests.mm
@@ -25,14 +25,14 @@
     return [NSData dataWithContentsOfURL:url];
 }
 
-class DataLoaderImpl: public DataLoader {
+class DataLoaderImpl final : public DataLoader {
 public:
     DataLoaderImpl(std::string filePath)
     :data_(read_data(filePath))
     {}
 
     Result<FreeableBuffer> load(
-        size_t offset, size_t size, __ET_UNUSED const DataLoader::SegmentInfo& segment_info) override {
+        size_t offset, size_t size, __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const override {
         NSData *subdata = [data_ subdataWithRange:NSMakeRange(offset, size)];
         return FreeableBuffer(subdata.bytes, size, nullptr);
     }
@@ -42,7 +42,7 @@
     }
 
 private:
-   NSData *data_;
+   NSData * const data_;
 };
 
 using Buffer = std::vector<uint8_t>;
diff --git a/examples/apple/coreml/executor_runner/main.mm b/examples/apple/coreml/executor_runner/main.mm
@@ -154,13 +154,13 @@ Args parse_command_line_args(NSArray<NSString *> *args) {
     return data;
 }
 
-class DataLoaderImpl: public DataLoader {
+class DataLoaderImpl final : public DataLoader {
 public:
     DataLoaderImpl(const std::string& filePath)
     :data_(read_data(filePath))
     {}
 
-    Result<FreeableBuffer> load(size_t offset, size_t size, __ET_UNUSED const DataLoader::SegmentInfo& segment_info) override {
+    Result<FreeableBuffer> load(size_t offset, size_t size, __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const override {
         NSData *subdata = [data_ subdataWithRange:NSMakeRange(offset, size)];
         return FreeableBuffer(subdata.bytes, size, nullptr);
     }
@@ -170,7 +170,7 @@ Args parse_command_line_args(NSArray<NSString *> *args) {
     }
 
 private:
-    NSData *data_;
+    NSData * const data_;
 };
 
 using Buffer = std::vector<uint8_t>;
diff --git a/extension/data_loader/buffer_data_loader.h b/extension/data_loader/buffer_data_loader.h
@@ -25,15 +25,15 @@ namespace util {
  * This can be used to wrap data that is directly embedded into the firmware
  * image, or to wrap data that was allocated elsewhere.
  */
-class BufferDataLoader : public DataLoader {
+class BufferDataLoader final : public DataLoader {
  public:
   BufferDataLoader(const void* data, size_t size)
       : data_(reinterpret_cast<const uint8_t*>(data)), size_(size) {}
 
   __ET_NODISCARD Result<FreeableBuffer> load(
       size_t offset,
       size_t size,
-      __ET_UNUSED const DataLoader::SegmentInfo& segment_info) override {
+      __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const override {
     ET_CHECK_OR_RETURN_ERROR(
         offset + size <= size_,
         InvalidArgument,
@@ -52,7 +52,7 @@ class BufferDataLoader : public DataLoader {
       size_t offset,
       size_t size,
       __ET_UNUSED const SegmentInfo& segment_info,
-      void* buffer) override {
+      void* buffer) const override {
     ET_CHECK_OR_RETURN_ERROR(
         buffer != nullptr,
         InvalidArgument,
diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp
@@ -122,7 +122,7 @@ void FreeSegment(void* context, void* data, __ET_UNUSED size_t size) {
 Result<FreeableBuffer> FileDataLoader::load(
     size_t offset,
     size_t size,
-    __ET_UNUSED const DataLoader::SegmentInfo& segment_info) {
+    __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const {
   ET_CHECK_OR_RETURN_ERROR(
       // Probably had its value moved to another instance.
       fd_ >= 0,
@@ -209,7 +209,7 @@ __ET_NODISCARD Error FileDataLoader::load_into(
     size_t offset,
     size_t size,
     __ET_UNUSED const SegmentInfo& segment_info,
-    void* buffer) {
+    void* buffer) const {
   ET_CHECK_OR_RETURN_ERROR(
       // Probably had its value moved to another instance.
       fd_ >= 0,
diff --git a/extension/data_loader/file_data_loader.h b/extension/data_loader/file_data_loader.h
@@ -25,7 +25,7 @@ namespace util {
  * Note that this will keep the file open for the duration of its lifetime, to
  * avoid the overhead of opening it again for every load() call.
  */
-class FileDataLoader : public DataLoader {
+class FileDataLoader final : public DataLoader {
  public:
   /**
    * Creates a new FileDataLoader that wraps the named file.
@@ -57,26 +57,26 @@ class FileDataLoader : public DataLoader {
         file_size_(rhs.file_size_),
         alignment_(rhs.alignment_),
         fd_(rhs.fd_) {
-    rhs.file_name_ = nullptr;
-    rhs.file_size_ = 0;
-    rhs.alignment_ = 0;
-    rhs.fd_ = -1;
+    const_cast<const char*&>(rhs.file_name_) = nullptr;
+    const_cast<size_t&>(rhs.file_size_) = 0;
+    const_cast<size_t&>(rhs.alignment_) = 0;
+    const_cast<int&>(rhs.fd_) = -1;
   }
 
   ~FileDataLoader() override;
 
   __ET_NODISCARD Result<FreeableBuffer> load(
       size_t offset,
       size_t size,
-      const DataLoader::SegmentInfo& segment_info) override;
+      const DataLoader::SegmentInfo& segment_info) const override;
 
   __ET_NODISCARD Result<size_t> size() const override;
 
   __ET_NODISCARD Error load_into(
       size_t offset,
       size_t size,
       __ET_UNUSED const SegmentInfo& segment_info,
-      void* buffer) override;
+      void* buffer) const override;
 
  private:
   FileDataLoader(
@@ -94,10 +94,10 @@ class FileDataLoader : public DataLoader {
   FileDataLoader& operator=(const FileDataLoader&) = delete;
   FileDataLoader& operator=(FileDataLoader&&) = delete;
 
-  const char* file_name_; // Owned by the instance.
-  size_t file_size_;
-  size_t alignment_;
-  int fd_; // Owned by the instance.
+  const char* const file_name_; // Owned by the instance.
+  const size_t file_size_;
+  const size_t alignment_;
+  const int fd_; // Owned by the instance.
 };
 
 } // namespace util
diff --git a/extension/data_loader/mmap_data_loader.cpp b/extension/data_loader/mmap_data_loader.cpp
@@ -149,7 +149,7 @@ void MunmapSegment(void* context, void* data, size_t size) {
 Result<FreeableBuffer> MmapDataLoader::load(
     size_t offset,
     size_t size,
-    __ET_UNUSED const DataLoader::SegmentInfo& segment_info) {
+    __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const {
   ET_CHECK_OR_RETURN_ERROR(
       // Probably had its value moved to another instance.
       fd_ >= 0,
diff --git a/extension/data_loader/mmap_data_loader.h b/extension/data_loader/mmap_data_loader.h
@@ -23,7 +23,7 @@ namespace util {
  * Note that this will keep the file open for the duration of its lifetime, to
  * avoid the overhead of opening it again for every load() call.
  */
-class MmapDataLoader : public DataLoader {
+class MmapDataLoader final : public DataLoader {
  public:
   /**
    * Describes how and whether to lock loaded pages with `mlock()`.
@@ -77,19 +77,19 @@ class MmapDataLoader : public DataLoader {
         page_size_(rhs.page_size_),
         fd_(rhs.fd_),
         mlock_config_(rhs.mlock_config_) {
-    rhs.file_name_ = nullptr;
-    rhs.file_size_ = 0;
-    rhs.page_size_ = 0;
-    rhs.fd_ = -1;
-    rhs.mlock_config_ = MlockConfig::NoMlock;
+    const_cast<const char*&>(rhs.file_name_) = nullptr;
+    const_cast<size_t&>(rhs.file_size_) = 0;
+    const_cast<size_t&>(rhs.page_size_) = 0;
+    const_cast<int&>(rhs.fd_) = -1;
+    const_cast<MlockConfig&>(rhs.mlock_config_) = MlockConfig::NoMlock;
   }
 
   ~MmapDataLoader() override;
 
   __ET_NODISCARD Result<FreeableBuffer> load(
       size_t offset,
       size_t size,
-      const DataLoader::SegmentInfo& segment_info) override;
+      const DataLoader::SegmentInfo& segment_info) const override;
 
   __ET_NODISCARD Result<size_t> size() const override;
 
@@ -111,11 +111,11 @@ class MmapDataLoader : public DataLoader {
   MmapDataLoader& operator=(const MmapDataLoader&) = delete;
   MmapDataLoader& operator=(MmapDataLoader&&) = delete;
 
-  const char* file_name_; // String data is owned by the instance.
-  size_t file_size_;
-  size_t page_size_;
-  int fd_; // Owned by the instance.
-  MlockConfig mlock_config_;
+  const char* const file_name_; // String data is owned by the instance.
+  const size_t file_size_;
+  const size_t page_size_;
+  const int fd_; // Owned by the instance.
+  const MlockConfig mlock_config_;
 };
 
 } // namespace util
diff --git a/extension/data_loader/shared_ptr_data_loader.h b/extension/data_loader/shared_ptr_data_loader.h
@@ -24,15 +24,15 @@ namespace util {
  *
  * This can be used to wrap data that was allocated elsewhere.
  */
-class SharedPtrDataLoader : public DataLoader {
+class SharedPtrDataLoader final : public DataLoader {
  public:
   SharedPtrDataLoader(std::shared_ptr<void> data, size_t size)
       : data_(data), size_(size) {}
 
   __ET_NODISCARD Result<FreeableBuffer> load(
       size_t offset,
       size_t size,
-      __ET_UNUSED const DataLoader::SegmentInfo& segment_info) override {
+      __ET_UNUSED const DataLoader::SegmentInfo& segment_info) const override {
     ET_CHECK_OR_RETURN_ERROR(
         offset + size <= size_,
         InvalidArgument,
diff --git a/runtime/core/data_loader.h b/runtime/core/data_loader.h
@@ -88,7 +88,7 @@ class DataLoader {
    * @returns a `FreeableBuffer` that owns the loaded data.
    */
   __ET_NODISCARD virtual Result<FreeableBuffer>
-  load(size_t offset, size_t size, const SegmentInfo& segment_info) = 0;
+  load(size_t offset, size_t size, const SegmentInfo& segment_info) const = 0;
 
   /**
    * Loads data from the underlying data source into the provided buffer.
@@ -108,7 +108,7 @@ class DataLoader {
       size_t offset,
       size_t size,
       const SegmentInfo& segment_info,
-      void* buffer) {
+      void* buffer) const {
     // Using a stub implementation here instead of pure virtual to expand the
     // data_loader interface in a backwards compatible way.
     (void)buffer;
diff --git a/runtime/executor/test/backend_integration_test.cpp b/runtime/executor/test/backend_integration_test.cpp
@@ -164,7 +164,7 @@ StubBackend StubBackend::singleton_;
  * A DataLoader that wraps a real DataLoader and records the operations
  * performed on it and the FreeableBuffers it loads.
  */
-class DataLoaderSpy : public DataLoader {
+class DataLoaderSpy final : public DataLoader {
  public:
   /// A record of an operation performed on this DataLoader.
   struct Operation {
@@ -178,8 +178,10 @@ class DataLoaderSpy : public DataLoader {
 
   explicit DataLoaderSpy(DataLoader* delegate) : delegate_(delegate) {}
 
-  Result<FreeableBuffer>
-  load(size_t offset, size_t size, const SegmentInfo& segment_info) override {
+  Result<FreeableBuffer> load(
+      size_t offset,
+      size_t size,
+      const SegmentInfo& segment_info) const override {
     Result<FreeableBuffer> buf = delegate_->load(offset, size, segment_info);
     if (!buf.ok()) {
       return buf.error();
@@ -268,7 +270,7 @@ class DataLoaderSpy : public DataLoader {
   /// The real loader to delegate to.
   DataLoader* delegate_;
 
-  std::vector<Operation> operations_;
+  mutable std::vector<Operation> operations_;
 };
 
 constexpr size_t kDefaultNonConstMemBytes = 32 * 1024;
