-
Notifications
You must be signed in to change notification settings - Fork 653
imagecache: remove redundant ImageSpec from ImageCache internals #4664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
imagecache: remove redundant ImageSpec from ImageCache internals #4664
Conversation
e4157c8 to
ecdbbcc
Compare
|
Hey @lgritz , I'm almost done with this PR, apologies for the massive changes, but I've reworked a bit the ownership of descriptors in the ImageCache. As we discussed offline, the final version consists in adding a standalone structure, internal to the private image cache, to describe or access a subset of the ImageSpec. One ImageSpec is allocated per SubimageInfo, and one Dimensions is allocated per LevelInfo if it differs from the ImageSpec at lip level 0. The owner of the allocated data is the Image cache file itself and it is all released when 'invalidate_file' is called with unique_ptr. The descriptors are reduced to a minimum if we reuse them across subimages and miplevels. I implemented a 'lazy' reuse scheme that checks up to the 10 previous subimages to find a matching descriptor. This can save up to 90% of memory for some Ptex files. Note that my tests reusing across all descriptors seems to save further up to 99% of the descriptors memory, but at that point the memory usage is dominated by the size of SubimageInfo and LevelInfo structures. The LevelInfo footprint has been reduced at its minimum so I can't really compress further and that's what dominates the current version; shared ImageSpec / LevelSpec takes few tens of mb while few millions of SubimageInfo / LevelInfo takes few hundreds of mb now. I think the PR is close to the final version, If you have some time, could you have a quick look at the code and let me know what you think ? |
4172a54 to
d864339
Compare
|
@lgritz I've updated the comment with what's in the PR, the tests are all green, the history is not clean but I wait for your input and the list of required changes before squashing all that together. |
3d239fb to
0a53f5f
Compare
83f33e4 to
aa1bb8d
Compare
378b655 to
329cd8c
Compare
* fix pvt::heapsize for raw pointers and std::vector<std::shared_ptr<T>> * add minified ImageSpec descriptor for miplevels, struct LevelSpec * move descriptors ownership to ImageCacheFile with unique_ptr * use raw pointers to access descriptors from SubimageInfo/LevelInfo * reduce LevelInfo footprint * deduplicate descriptors across subimages and mip levels * rework ImageCacheFile::open to create immutable descriptors Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
* compression/decompression is slower the regular access * does not significantly save memory, when descriptors deduplication is used * result: simplify access to descriptors, files faster to open Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
* remove struct LevelSpec and introduce struct ImageSpec::Dimensions * union ImageSpec::Dimensions and the original members in ImageSpec * change the accessors accordingly * unfortunately this breaks the ABI Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
* move struct Dimensions to ImageCacheFile internals * introduce Dimensions::convert to create a view onto the ImageSpec members * hugely simplify access to the descriptors Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
…e; preallocate the LevelInfo buffers when possible; if there are a lot of descriptor, shrink the buffers to size after opening file; Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
… onto ImageSpec Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
ccff961 to
96861b8
Compare
| //! Add the number of miplevels as an attribute for the first miplevel. | ||
| //! TOFIX: adding the following attribute breaks unit tests | ||
| // if (m_miplevel == 0 && part.nmiplevels > 1) | ||
| // m_spec.attribute("oiio:miplevels", part.nmiplevels); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it only break unit tests by printing the "oiio:miplevels" in certain test output and we just need to update that test output reference? Do you need exr files to do this trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to break more than the just output, i.e. for Ptex I just had to update the output text and the test passed again, for Exr it is beyond my understanding so far..
|
Sorry it's taken me a while to get to this. My comments are extremely minor, and other than that, this all LGTM. Make those minor changes and rebase to the current main (there seems to be some minor conflict) and then I am ready to merge it. |
Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
…pec-removal Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
|
Thanks for the review @lgritz , I implemented your feedbacks in the last commits and fixed the conflicts, I think it is good to go ! |
lgritz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the edits
Oops, my bad. Looks like AcademySoftwareFoundation#4664 (merged about a week ago) and AcademySoftwareFoundation#4783 (merged today, but authored before AcademySoftwareFoundation#4664 was merged) interacted in a way that produced a build break. This tiny fix is essential to unbreak it. Signed-off-by: Larry Gritz <lg@larrygritz.com>
…demySoftwareFoundation#4664) Backend changes for issue AcademySoftwareFoundation#4436. ## Description - add `ImageDims` struct to store only a subset of `ImageSpec` or use it as a view on an existing ImageSpec. - move `ImageDims` and `ImageSpec` ownership to `ImageCacheFile` and use them as a pool - add `ImageSpec* m_spec` in `SubimageInfo` to access the descriptors that are common across all mip level - add `ImageDims* m_dims` in `LevelInfo` to access the despcriptors that differ per mip level - add functions to access to mip level dimensions: ```c++ const Dimensions& SubimageInfo::leveldims(int miplevel) const; ``` - do the plumbing across all `libtexture` to use the new interface ## Tests I performed a batch of testing to compare the memory usage and performance of different strategies using 17 `ptex` files from the PTex library samples (some from the Moana Island dataset, so they are quite heavy). The baseline numbers are approximates but gives a good estimate of the memory usage. Baseline | Total (MB) | Specs (MB) (%) | # specs | Subimages (MB) (%) -- | -- | -- | -- | -- OIIO 3.0 main | 1500 | 1200 (80%) | 4936364 | 300 (20%) Method | Total (MB) | Specs (MB) (%) | # specs | Subimages (MB) (%) -- | -- | -- | -- | -- No reuse | 787,2 | 454,3 (58%) | 4936381 | 332,5 (42%) Reuse dist=1 | 442,2 | 109,3 (25%) | 1193485 | 332,5 (75%) Reuse dist=10 | 368,5 | 35,6 (10%) | 380506 | 332,5 (90%) Reuse dist=inf | 333,2 | 0,292 (<1%) | 3020 | 332,5 (>99%) These numbers shows that the memory is now bounded by the `SubimageInfo` + `LevelInfo` structure sizes, and not from the descriptors `ImageSpec` + `Dimensions` with the right amount of reuse across subimages and mip levels. --------- Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
…ation#4794) Oops, my bad. Looks like AcademySoftwareFoundation#4664 (merged about a week ago) and AcademySoftwareFoundation#4783 (merged today, but authored before AcademySoftwareFoundation#4664 was merged) interacted in a way that produced a build break. This tiny fix is essential to unbreak it. Signed-off-by: Larry Gritz <lg@larrygritz.com>
…demySoftwareFoundation#4664) Backend changes for issue AcademySoftwareFoundation#4436. ## Description - add `ImageDims` struct to store only a subset of `ImageSpec` or use it as a view on an existing ImageSpec. - move `ImageDims` and `ImageSpec` ownership to `ImageCacheFile` and use them as a pool - add `ImageSpec* m_spec` in `SubimageInfo` to access the descriptors that are common across all mip level - add `ImageDims* m_dims` in `LevelInfo` to access the despcriptors that differ per mip level - add functions to access to mip level dimensions: ```c++ const Dimensions& SubimageInfo::leveldims(int miplevel) const; ``` - do the plumbing across all `libtexture` to use the new interface ## Tests I performed a batch of testing to compare the memory usage and performance of different strategies using 17 `ptex` files from the PTex library samples (some from the Moana Island dataset, so they are quite heavy). The baseline numbers are approximates but gives a good estimate of the memory usage. Baseline | Total (MB) | Specs (MB) (%) | # specs | Subimages (MB) (%) -- | -- | -- | -- | -- OIIO 3.0 main | 1500 | 1200 (80%) | 4936364 | 300 (20%) Method | Total (MB) | Specs (MB) (%) | # specs | Subimages (MB) (%) -- | -- | -- | -- | -- No reuse | 787,2 | 454,3 (58%) | 4936381 | 332,5 (42%) Reuse dist=1 | 442,2 | 109,3 (25%) | 1193485 | 332,5 (75%) Reuse dist=10 | 368,5 | 35,6 (10%) | 380506 | 332,5 (90%) Reuse dist=inf | 333,2 | 0,292 (<1%) | 3020 | 332,5 (>99%) These numbers shows that the memory is now bounded by the `SubimageInfo` + `LevelInfo` structure sizes, and not from the descriptors `ImageSpec` + `Dimensions` with the right amount of reuse across subimages and mip levels. --------- Signed-off-by: Basile Fraboni <basile.fraboni@gmail.com>
…ation#4794) Oops, my bad. Looks like AcademySoftwareFoundation#4664 (merged about a week ago) and AcademySoftwareFoundation#4783 (merged today, but authored before AcademySoftwareFoundation#4664 was merged) interacted in a way that produced a build break. This tiny fix is essential to unbreak it. Signed-off-by: Larry Gritz <lg@larrygritz.com>
First draft of backend changes for issue #4436.
Description
ImageDimsstruct to store only a subset ofImageSpecor use it as a view on an existing ImageSpec.ImageDimsandImageSpecownership toImageCacheFileand use them as a poolImageSpec* m_specinSubimageInfoto access the descriptors that are common across all mip levelImageDims* m_dimsinLevelInfoto access the despcriptors that differ per mip levellibtextureto use the new interfaceTests
I performed a batch of testing to compare the memory usage and performance of different strategies using 17
ptexfiles from the PTex library samples (some from the Moana Island dataset, so they are quite heavy). The baseline numbers are approximates but gives a good estimate of the memory usage.These numbers shows that the memory is now bounded by the
SubimageInfo+LevelInfostructure sizes, and not from the descriptorsImageSpec+Dimensionswith the right amount of reuse across subimages and mip levels.Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).