Skip to content
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

APPLE: Fix for Issue #1749 #2089

Closed
wants to merge 0 commits into from

Conversation

creijon
Copy link
Contributor

@creijon creijon commented Nov 14, 2022

Description of Change(s)

The _topology field in the drawingcoord was overflowing the 8-bit range for assets with a large number of geomsubsets. This has been promoted to a 32-bit value to avoid this.

The instancePrimvar field could also overflow, so this has been promoted to a 32 bit value too.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@creijon creijon changed the title Fix for Issue Fix for Issue #1749 Nov 14, 2022
@creijon creijon changed the title Fix for Issue #1749 APPLE: Fix for Issue #1749 Nov 14, 2022
@spiffmon
Copy link
Member

I'm not up to date on current architecture, but am curious, @jonny-apple , if this changes the size of the structure, when packed? If so, it would be great to see some memory numbers with large numbers of meshes?

@creijon
Copy link
Contributor Author

creijon commented Nov 15, 2022

I'm not up to date on current architecture, but am curious, @jonny-apple , if this changes the size of the structure, when packed? If so, it would be great to see some memory numbers with large numbers of meshes?

Very good point spiff, I'll run some numbers on the large stages that we have as test assets.

@sunyab
Copy link
Contributor

sunyab commented Nov 19, 2022

Filed as internal issue #USD-7774

@creijon
Copy link
Contributor Author

creijon commented Nov 19, 2022

@spiffmon , I've run a few variations of the field sizes in the HdDrawingCoord structure.
I've used the TS4 assets that you gave us, which has 456,356 prims.
These figures were from running usdview on a Mac with latest OS. It takes a little bit of time for the memory to settle down, so there is some variability.

Firstly, the original 8 bits per field
(9 bytes total, probably rounded to 12)

Physical footprint: 4.7G
Physical footprint (peak): 5.6G

Physical footprint: 4.8G
Physical footprint (peak): 5.6G

Physical footprint: 4.6G
Physical footprint (peak): 5.6G

Then 16 bits for _topology and _instancePrimvar
(11 bytes total, probably rounded to 12)

Physical footprint: 4.7G
Physical footprint (peak): 5.8G

Physical footprint: 4.6G
Physical footprint (peak): 5.6G

Physical footprint: 4.6G
Physical footprint (peak): 5.8G

Then 32 bits for _topology and _instancePrimvar
(15 bytes total, probably rounded to 16)
Physical footprint: 4.7G
Physical footprint (peak): 5.5G

Physical footprint: 4.8G
Physical footprint (peak): 5.5G

Physical footprint: 4.8G
Physical footprint (peak): 5.6G

It looks like this has a small impact on the overall memory size, but if there is a concern then 16 bits for these fields gives us enough headroom and shouldn't change the memory footprint.

I'll amend the PR to use 16 bits.

@spiffmon
Copy link
Member

Thanks so much, @jonny-apple - that seems like a good choice, for now. I'll note that we can double the value of our money by using uint_16t for those two fields that we specifically want headroom with?

@drwave
Copy link

drwave commented Nov 20, 2022 via email

@creijon creijon force-pushed the jon/dev/fix_1749 branch 4 times, most recently from 7ba8e23 to 4105a15 Compare November 25, 2022 09:53
@creijon creijon force-pushed the jon/dev/fix_1749 branch 2 times, most recently from 8bf345a to 1c0445c Compare December 3, 2022 06:58
pixar-oss added a commit that referenced this pull request Dec 9, 2022
APPLE: Fix for Issue #1749

(Internal change: 2256327)
@creijon creijon closed this Dec 10, 2022
@creijon creijon deleted the jon/dev/fix_1749 branch December 10, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants