Skip to content

Commit

Permalink
Mark thread-safe methods of DataLoader as const.
Browse files Browse the repository at this point in the history
Differential Revision: D61406229

Pull Request resolved: pytorch#4756
  • Loading branch information
shoumikhin committed Aug 16, 2024
1 parent b75e7d7 commit 622de2d
Show file tree
Hide file tree
Showing 10 changed files with 45 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -42,7 +42,7 @@
}

private:
NSData *data_;
NSData * const data_;
};

using Buffer = std::vector<uint8_t>;
Expand Down
6 changes: 3 additions & 3 deletions examples/apple/coreml/executor_runner/main.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -170,7 +170,7 @@ Args parse_command_line_args(NSArray<NSString *> *args) {
}

private:
NSData *data_;
NSData * const data_;
};

using Buffer = std::vector<uint8_t>;
Expand Down
6 changes: 3 additions & 3 deletions extension/data_loader/buffer_data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions extension/data_loader/file_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
22 changes: 11 additions & 11 deletions extension/data_loader/file_data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion extension/data_loader/mmap_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
24 changes: 12 additions & 12 deletions extension/data_loader/mmap_data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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()`.
Expand Down Expand Up @@ -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;

Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions extension/data_loader/shared_ptr_data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions runtime/core/data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand Down
10 changes: 6 additions & 4 deletions runtime/executor/test/backend_integration_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 622de2d

Please sign in to comment.