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

Fix FName memory issue when running for extended periods of time #1423

Merged
merged 3 commits into from
May 29, 2024

Conversation

csciguy8
Copy link
Contributor

Closes #1422.

A user reported an error with FName allocations when running for long periods of time (5 hours).

Interesting output from Unreal
FName overflow, allocated 1024MB of string data. FName strings are never freed and should be created sparingly

In loadPrimitiveGameThreadPart we create a new FName object for every primitive loaded. In some cases, these can be fairly large (Ex. 165 characters) as they are essentially the url to the asset that was fetched.

This PR solves the problem by avoiding creating an FName entirely.

Ran through all the samples level and didn't see any issues. Also, these meshes and components are not visible in the Outliner window, so there's no apparent user facing change either.

@csciguy8
Copy link
Contributor Author

so there's no apparent user facing change either.

One use for named components is for debugging, specifically when wanting to know what assets are mapped to what you are actually looking at. (thanks @kring for mentioning)

Here's how... click on a piece of the terrain in the viewport, try to delete it. You'll see a console message like this...

Cmd: DELETE Cmd: ACTOR DELETE LogEditorActor: Cannot delete component CesiumGltfPrimitiveComponent /Game/CesiumSamples/Maps/01/01_CesiumWorld.01_CesiumWorld:PersistentLevel.CesiumWorldTerrain.CesiumGltfComponent_282.https://assets.ion.cesium.com/us-east-1/asset_depot/1/CesiumWorldTerrain/v1.2/16/20975/46531.terrain?v=1.2.0&extensions=octvertexnormals-metadata-watermask mesh 0 primitive 0: Can't delete non-editable component.

Is this valuable enough to keep the FName creation around? Probably not. And if debugging in this way, you can just make a test branch, and put the naming back in anyway. Interesting though.

@csciguy8 csciguy8 requested a review from azrogers May 28, 2024 20:28
@azrogers
Copy link
Contributor

I think this suggestion was made a bit jokingly, but I do think it would be good to wrap this in a define. Something like CESIUM_CREATE_TILE_NAMES. Would mean that future developers don't have to hunt through old pull requests to figure out how to enable this debugging feature.

@csciguy8
Copy link
Contributor Author

@azrogers Sensible suggestion. I've made this change, and it's ready for another look.

@azrogers
Copy link
Contributor

Looks good to me. Thanks @csciguy8!

@azrogers azrogers merged commit bb22904 into main May 29, 2024
25 checks passed
@azrogers azrogers deleted the dont-name-components branch May 29, 2024 19:58
@j9liu j9liu mentioned this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants