Enable mmap for reading model from cache #26696

olpipi · 2024-09-19T16:33:14Z

Details:

Enable mmap for reading model from cache

Tickets:

CVS-152423

praasz · 2024-09-20T07:58:25Z

src/inference/src/cache_manager.hpp

@@ -78,7 +80,7 @@ class ICacheManager {
     * @param id Id of cache (hash of the model)
     * @param reader Lambda function to be called when input stream is created


Add new parameter to doxy comment.

there is not new parameter anymore

The parameter bool enable_mmap is still here (should it be removed?), sync doxy comment with function signature.

praasz · 2024-09-20T08:15:01Z

src/inference/src/dev/core_impl.cpp

    ov::SoPtr<ov::ICompiledModel> compiled_model;
    struct HeaderException {};

    OPENVINO_ASSERT(cacheContent.cacheManager != nullptr);
    try {
-        cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, [&](std::istream& networkStream) {
+        cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, enable_mmap, [&](std::istream& networkStream) {


Suggested change

cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, enable_mmap, [&](std::istream& networkStream) {

cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, coreConfig.get_enable_mmap(), [&](std::istream& networkStream) {

The additional parameter enable_mmap can be removed as this member can access core configuration.

Unfortunately no. load_model_from_cache is a static method and we cannot get coreConfig.
But why is it static?
@ilya-lavrenov @praasz. Since now this method uses CoreImpl internal state. So can we make it not static? Is there any objections to it?

I changed it to non static const method. Please comment if it not ok

The core_impl is internal implementation. There shouldn't be any issue to adjust it to what is required.

praasz · 2024-09-20T08:26:36Z

src/core/dev_api/openvino/runtime/shared_buffer.hpp

+    SharedStreamBuffer() = delete;
+
+private:
+    T _shared_object;


Maybe this class has to not be so generic and shared object can be stored as shared pointer to void.
example

class SharedStreamBuffer final : public std::stringbuf { public: SharedStreamBuffer(char* data, size_t size, std::shared_ptr<void> shared_object) : m_shared_object(std::move(shared_object)) { basic_streambuf::pubsetbuf(data, size); } SharedStreamBuffer() = delete; private: std::shared_ptr<void> m_shared_object; };

I changed it. Now it creates (in RAII manner) SharedBuffer, which already has an implementation to own the shared object

ilya-lavrenov · 2024-09-20T11:55:46Z

src/inference/src/dev/core_impl.cpp

@@ -751,7 +751,7 @@ ov::SoPtr<ov::ICompiledModel> ov::CoreImpl::compile_model(const std::shared_ptr<
                                           parsed._config,
                                           ov::SoPtr<ov::IRemoteContext>{},
                                           cacheContent);
-        });
+        }, coreConfig.get_enable_mmap());


what if config contains enable_mmap(false) while coreConfig has true for the same key?

Lets read it always from config. It is true by default

praasz · 2024-09-23T05:07:35Z

src/inference/src/cache_manager.hpp

@@ -78,7 +80,7 @@ class ICacheManager {
     * @param id Id of cache (hash of the model)
     * @param reader Lambda function to be called when input stream is created


The parameter bool enable_mmap is still here (should it be removed?), sync doxy comment with function signature.

praasz · 2024-09-23T06:57:58Z

src/core/dev_api/openvino/runtime/shared_buffer.hpp

@@ -28,4 +28,51 @@ class SharedBuffer : public ov::AlignedBuffer {
    T _shared_object;
 };

+/// \brief SharedStreamBuffer class to store pointer to pre-acclocated buffer and provide streambuf interface.
+class SharedStreamBuffer : public std::streambuf {


Consider:

put this buffer in separate file like string buffer

add OPENVINO_API

split implementation to hpp/cpp

add some unit test (like for string buffer) to test overridden function how they are used.

Just another alternative to integrate it with ov buffer interface.
If multiple inheritance is an issue the composition can be used.

class SharedStreamBuffer : public SharedBuffer<std::shared_ptr<void>>, public std::streambuf { public: SharedStreamBuffer(char* data, size_t size, const std::shared_ptr<void>& shared_object) : SharedBuffer<std::shared_ptr<void>>(data, size, shared_object), std::streambuf(), m_offset(0) {} protected: std::streamsize xsgetn(char* s, std::streamsize count) override { auto real_count = std::min<std::streamsize>(showmanyc(), count); std::memcpy(s, get_ptr(m_offset), real_count); m_offset += real_count; return real_count; } int_type underflow() override { return (size() == m_offset) ? traits_type::eof() : traits_type::to_int_type(*(get_ptr<const char>() + m_offset)); } int_type uflow() override { return (size() == m_offset) ? traits_type::eof() : traits_type::to_int_type(*(get_ptr<const char>() + m_offset++)); } std::streamsize showmanyc() override { return size() - m_offset; } private: size_t m_offset; };

praasz · 2024-09-23T06:59:46Z

src/core/dev_api/openvino/runtime/shared_buffer.hpp

+    }
+
+protected:
+    std::shared_ptr<AlignedBuffer> m_alligned_buffer;


Why shared pointer is required. The ov::SharedBuffer will manage owner ship
Why use aligned buffer not shared?

Suggested change

std::shared_ptr<AlignedBuffer> m_alligned_buffer;

SlignedBuffer m_buffer;

Should be sufficient

praasz · 2024-09-23T07:00:56Z

src/core/dev_api/openvino/runtime/shared_buffer.hpp

+        : SharedStreamBuffer(data, size),
+          m_alligned_buffer(std::make_shared<SharedBuffer<T>>(data, size, shared_object)) {}
+
+    std::shared_ptr<AlignedBuffer> get_aligned_buffer() {


Why is required and why not just return internal buffer. When shared buffer used it will just increment counter in stored shared object.

nshchego · 2024-10-08T06:07:05Z

src/core/dev_api/openvino/runtime/shared_buffer.hpp

+/// \brief SharedStreamBuffer class to store pointer to pre-acclocated buffer and provide streambuf interface.
+class SharedStreamBuffer : public std::streambuf {
+public:
+    SharedStreamBuffer(char* data, size_t size) : m_data(data), m_size(size), m_offset(0) {}


'data' is not passed to std::streambuf, so operator>> for std::istream should not work in core_impl and HeaderException should be thrown. Actually, it could be done on Linux only, due to the std::streambuf has different implementations depend on OS.
Another way is to define operator>> but you should handle end of line for different OS at least.

olpipi requested review from a team as code owners September 19, 2024 16:33

github-actions bot added category: inference OpenVINO Runtime library - Inference category: Core OpenVINO Core (aka ngraph) labels Sep 19, 2024

olpipi force-pushed the inport_model_with_mmap branch 2 times, most recently from 8095ad4 to 57a4efa Compare September 19, 2024 16:34

ilya-lavrenov self-assigned this Sep 19, 2024

ilya-lavrenov requested a review from nshchego September 19, 2024 18:55

praasz reviewed Sep 20, 2024

View reviewed changes

praasz self-assigned this Sep 20, 2024

ilya-lavrenov reviewed Sep 20, 2024

View reviewed changes

olpipi requested review from ilya-lavrenov and praasz September 20, 2024 15:51

olpipi force-pushed the inport_model_with_mmap branch 2 times, most recently from e73aa8e to 216a980 Compare September 20, 2024 16:02

praasz reviewed Sep 23, 2024

View reviewed changes

olpipi added 5 commits October 7, 2024 14:43

Enable mmap for reading model from cache

e09a70a

Update sahred buffer

8ff983b

Apply comments

471fde6

Fix clang-format

d6d518b

fix

385d657

olpipi force-pushed the inport_model_with_mmap branch from 769fbac to 09904dc Compare October 7, 2024 15:23

olpipi requested review from a team as code owners October 7, 2024 15:23

github-actions bot added the category: GPU OpenVINO GPU plugin label Oct 7, 2024

Enable mmap only for gpu

4d082b2

olpipi force-pushed the inport_model_with_mmap branch from 09904dc to 4d082b2 Compare October 7, 2024 15:42

nshchego reviewed Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable mmap for reading model from cache #26696

Enable mmap for reading model from cache #26696

olpipi commented Sep 19, 2024 •

edited

Loading

praasz Sep 20, 2024

olpipi Sep 20, 2024

praasz Sep 23, 2024

praasz Sep 20, 2024

olpipi Sep 20, 2024 •

edited

Loading

olpipi Sep 20, 2024

praasz Sep 23, 2024

praasz Sep 20, 2024

olpipi Sep 20, 2024 •

edited

Loading

ilya-lavrenov Sep 20, 2024

olpipi Sep 20, 2024

praasz Sep 23, 2024

praasz Sep 23, 2024

praasz Sep 23, 2024

praasz Sep 23, 2024

nshchego Oct 8, 2024

		@@ -78,7 +80,7 @@ class ICacheManager {
		* @param id Id of cache (hash of the model)
		* @param reader Lambda function to be called when input stream is created

	cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, enable_mmap, [&](std::istream& networkStream) {
	cacheContent.cacheManager->read_cache_entry(cacheContent.blobId, coreConfig.get_enable_mmap(), [&](std::istream& networkStream) {

	std::shared_ptr<AlignedBuffer> m_alligned_buffer;
	SlignedBuffer m_buffer;

Enable mmap for reading model from cache #26696

Are you sure you want to change the base?

Enable mmap for reading model from cache #26696

Conversation

olpipi commented Sep 19, 2024 • edited Loading

Details:

Tickets:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpipi Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpipi Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olpipi commented Sep 19, 2024 •

edited

Loading

olpipi Sep 20, 2024 •

edited

Loading

olpipi Sep 20, 2024 •

edited

Loading