performance: reduce the # of open file handles #298
Conversation
Installing thousands of bundles within a process can cause undefined behavior when the OS open file limit has been reached. If the bundle only contains a manifest.json, close the file after reading the manifest.json contents. If the bundle meta-data is read again, open the file and leave it open until the bundle is uninstalled. Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
| @@ -25,6 +25,7 @@ | |||
|
|
|||
| #include "miniz.h" | |||
|
|
|||
| #include <atomic> | |||
There was a problem hiding this comment.
Think you don't need this anymore
Codecov Report
@@ Coverage Diff @@
## development #298 +/- ##
===============================================
+ Coverage 84.21% 84.31% +0.09%
===============================================
Files 94 94
Lines 6539 6999 +460
===============================================
+ Hits 5507 5901 +394
- Misses 1032 1098 +66
|
|
@CppMicroServices/developers please review as soon as possible. I'd like to submit this within a couple weeks if there are no significant issues. Thanks! |
Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
There was a problem hiding this comment.
I believe we had a discussion about opening / closing the bundle resource container on demand, if a resource is actually requested. But I cannot find a record of that conversation.
The changes are definitely good and should be merged. We can further improve this later. However, the manifest file checking in combination with the more recent heap allocation PR #306 probably needs another look (in the context of the PR #306 .
| mz_zip_reader_end(&m_ZipArchive); | ||
| try { | ||
| CloseContainer(); | ||
| } catch(...) {} |
There was a problem hiding this comment.
I suggest to not use ... in catch clauses. If SEH on Windows is enabled, this will swallow hardware exceptions and continuing is usually not a good idea.
There was a problem hiding this comment.
ok. i'll change ... to const std::exception&
| // if the only resource is the manifest file. On this assumption, | ||
| // close the open file handle to the zip file to improve performance | ||
| // and avoid exceeding OS open file handle limits. | ||
| if (OnlyContainsManifest(location)) { |
There was a problem hiding this comment.
In combination with the heap allocation PR, this would copy the zip blob into memory, just for reading the central directory record and check the entry names. I think we should try and do better in such cases.
There was a problem hiding this comment.
Yes. PR #306 needs to be re-factored after this PR is merged.
|
@saschazelzer we did have a conversation about opening / closing the bundle resource container on demand in issue #299. |
|
@jeffdiclemente Thanks for pointing this out. What is the strategy then? Are you currently looking into changing how the resource container manages file handles or do you rather want to merge this now as is and go ahead with PR #306 ? |
|
@saschazelzer I'd like to merge this PR now and I'll create a follow up issue to change how the resource container manages file handles. |
@jeffdiclemente Sounds good to me. |
Fixes CppMicroServices#299 Installing thousands of bundles within a process can cause undefined behavior when the OS open file limit has been reached. If the bundle only contains a manifest.json, close the file after reading the manifest.json contents. If the bundle meta-data is read again, open the file and leave it open until the bundle is uninstalled. Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>
Installing thousands of bundles within a process can cause undefined behavior if the OS open file limit has been reached. In our use case, we reached this open file limit on Windows.
In order to resolve the issue on Windows and to allow thousands of bundles to be installed into an applications, this solution will close the file handle for the bundle if it only contains a manifest file. The idea is that with only a manifest.json file, clients are unlikely to use another CppMicroServices resource API to read meta-data from the .zip file. If the resource API is used after the bundle is installed, the file will remain open until the bundle is uninstalled.
There are most likely more improvements that can be made to manage the # of file handles CppMicroServices keeps open and this is a small step towards getting to that state.