Reduce IR Constant Memory Retention - #35457
Conversation
|
|
||
| const auto separate_const_weights_loading = use_separate_const_weights_loading() && !weights; | ||
| if (separate_const_weights_loading) { | ||
| weights_provider = std::make_shared<ov::util::FileWeightsProvider>(weights_path); |
There was a problem hiding this comment.
The same use should be if input is stream not only for path
| std::filesystem::path m_weights_path; | ||
| size_t m_weights_size = 0; | ||
| size_t m_weights_source_id = 0; | ||
| std::shared_ptr<ov::AlignedBuffer> m_weights_source_handle; |
There was a problem hiding this comment.
It keeps the data source buffer + buffer in the map.
Is there no data duplication?
Will this provider will not live too long that data buffers set in Constant node will be not blocked to be removed if Constant node removed?
| size_t m_weights_source_id = 0; | ||
| std::shared_ptr<ov::AlignedBuffer> m_weights_source_handle; | ||
| // Cache of previously loaded weights regions, keyed by (offset, size) of the region in the weights file. | ||
| std::map<WeightsRegionKey, std::shared_ptr<ov::AlignedBuffer>> m_loaded_weights_regions; |
There was a problem hiding this comment.
For futher analysis why not use Constant ID (buffer) for it?
| } | ||
|
|
||
| std::shared_ptr<ov::AlignedBuffer> FileWeightsProvider::make_region(size_t offset, size_t size) { | ||
| OPENVINO_ASSERT(offset <= m_weights_size && size <= m_weights_size - offset, "Incorrect weights in bin file!"); |
There was a problem hiding this comment.
Looks like common validation pattern, should some helper be created to return bool if offset, size are valid?
| std::ifstream weights_stream(m_weights_path, std::ios::binary); | ||
| OPENVINO_ASSERT(weights_stream.is_open(), m_weights_path, " cannot be opened"); |
There was a problem hiding this comment.
For analysis, should be opened each time and checked?
Or provider can keep it open?
The provider should be removed when frontend complete model parsing
| if (m_weights->size() < offset + size) | ||
| OPENVINO_ASSERT(m_weights_provider, "Empty weights data in bin file or bin file cannot be found!"); | ||
|
|
||
| if (m_weights_provider->size() < offset + size) |
There was a problem hiding this comment.
The check is done here and similar in provider are both required?
| const size_t offset = item.second.second; | ||
| const char* data = weights->get_ptr<char>() + offset; | ||
| auto buffer = weights_provider->make_region(offset, item.second.first); | ||
| const char* data = buffer->get_ptr<char>(); |
There was a problem hiding this comment.
Is it required, the const accept void*, or make_region/buffer is aligned buffer const also accept such buffer
olpipi
left a comment
There was a problem hiding this comment.
I'd recommend to write tests with weight_provder mocks to check if they are really used as planned
| size_t m_weights_source_id = 0; | ||
| std::shared_ptr<ov::AlignedBuffer> m_weights_source_handle; | ||
| // Cache of previously loaded weights regions, keyed by (offset, size) of the region in the weights file. | ||
| std::map<WeightsRegionKey, std::shared_ptr<ov::AlignedBuffer>> m_loaded_weights_regions; |
There was a problem hiding this comment.
[MEDIUM] you only add new buffers into m_loaded_weights_regions but never clean unused buffers. I'd suggest to store them as weak_ptr and clean expired ones regularly.
| std::map<WeightsRegionKey, std::shared_ptr<ov::AlignedBuffer>> m_loaded_weights_regions; | |
| std::map<WeightsRegionKey, std::weak_ptr<ov::AlignedBuffer>> m_loaded_weights_regions; |
…e WeightsProvider for weight source management
|
|
||
| size_t get_mmap_region_threshold() { | ||
| const auto page_size = ov::util::get_system_page_size(); | ||
| return page_size > 0 ? static_cast<size_t>(page_size) : 4096; |
There was a problem hiding this comment.
Why use some value if page size is 0? Will it be even possible?
Set threshold as 4k can be too small still a lot of overhead can be created.
Lets apply same logic as for ONNX FE where 1M is used . @t-jankowski could you check recommend threshold value?
| if (!m_weights_path.empty()) | ||
| model->get_rt_info()["__weights_path"] = ov::util::path_to_string(m_weights_path); | ||
| parse_pre_process(m_root, m_weights, model); | ||
| if (m_weights_provider) { |
There was a problem hiding this comment.
For future improvement, this check should not be required as weight is mandatory
Details:
Tickets:
AI Assistance: