search

RenderKit / oidn

Intel® Open Image Denoise library
https://www.openimagedenoise.org/
Apache License 2.0
1.74k stars 160 forks source link

The OpenImageDenoise weights end up in the Read/Write PE section instead of ReadOnly #128

Closed dmachaj closed 2 years ago

dmachaj commented 2 years ago

I happened to be taking a look at the binary size for Blender.exe on Windows and I noticed that Blender has a massive 50MB of .data in the executable. Upon closer inspection the largest of these read/write data sections are the OpenImageDenoise weights. Blender pulls in the weights as a static library so they seem like they should be in the .rdata (read-only data) section. There are performance benefits to the .rdata section over the .data section because immutability is a good thing. It is my hope that sprinkling const or constexpr in a few places is sufficient to address this issue and improve the performance of all consumers of this project.

I performed analysis using SizeBench.

image

atafra commented 2 years ago

Thanks, this has been fixed in devel. However, I don't think this actually affects performance in any noticeable way in practice.

dmachaj commented 2 years ago

Thank you for fixing this :)

As I understand it there are two benefits to this change on Windows. The first is that readonly pages should be slightly more performant to load compared to read/write pages. The biggest benefit is that readonly pages can be shared across processes. If there are multiple processes accessing the same data pages then that is potentially 50MB per process of reduced memory usage thanks to the sharing. It is unclear to me whether multiple processes will ever access this same library at the same time in real usage. Nonetheless that's a nice potential benefit in exchange for adding const in a few places :).