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

Update Magnum projects and switch to CgltfImporter from TinyGltfImporter #1549

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Oct 25, 2021

Motivation and Context

This brings mainly a switch to CgltfImporter from TinyGltfImporter, which should be a drop-in replacement with equivalent functionality but vastly improved efficiency. More below. Besides that, there's a few web-focused improvements to multidraw functionality

CgltfImporter

The plots below show parsing of about 600 materials from a half-gigabyte file using magnum-sceneconverter --info-materials --profile. (Importing meshes is mostly about dealing with the data blobs, where the work doesn't really differ between the two plugins, so that's not shown here.)

image

The new plugin is built on top of a library that not only gives Magnum full control over how data are loaded into memory (unlike TinyGltf, which copied everything into its memory), but also is capable of parsing the JSON in-place, without copying its contents to its own structures. That results in pretty nice implicit memory savings. The second vs third row in the two plots shows the difference between TinyGltfImporter and CgltfImporter JSON parsing speed and memory usage given the same conditions. But wait, there's more!

Because the library no longer populates its own internal state with a copy of the input data, this finally enables a workflow where the importer can be told to operate directly on an externally owned memory using a new openMemory() API. That's the fourth row in the plots, and the saved copy is a pretty significant saving on its own.

And finally, memory-mapping a file and opening it using openMemory() leads to only the actually touched parts of the file being paged into memory. In case of the 439.5 MB file, it only needed to read and parse the JSON, which resulted in just 6.7 MB of actual physical memory being needed. (Yes, similar memory savings could be made if the file was read byte-by-byte and seeked as appropriate, but that's quite labor intensive and with all the filesystem calls it wouldn't get anywhere near the speed of just accessing a memory-mapped file.)

image

Note that, in order to make the PR as non-invasive as possible, I didn't change any openFile() calls to mmap + openMemory() -- that's something the resource amanger has to be aware of, ensuring the memory-mapped file doesn't get close for as long as the importer is used.

The final step in this direction (and what gets used by the batch-rendering-friendly import pipeline) is about making also MeshData imports zero-copy. In short, instead of the importer returning a MeshData with a copy of given mesh data, it would directly reference the (memory-mapped) file that got passed to openMemory(). Most of the work is already in place, the remaining bits are happening in mosra/magnum#240.

How Has This Been Tested

⚠️ All tests pass for me on my side, but since this completely swaps out the existing glTF import pipeline for another, I need a handful of confirmations that everything indeed behaves the same as before. Especially with large datasets and real-world training use cases. That's why it's also marked as a draft, to avoid accidental merges ;)

Types of changes

  • Docs change / refactoring / dependency upgrade

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 25, 2021
Copy link
Contributor

@jturner65 jturner65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this with the ReplicaCAD 1.1 dataset through the c++ viewer and it seemed to work just fine.

@Skylion007
Copy link
Contributor

It worked fine for me as well. @jturner65 Do we have any benchmarks in how much faster it loaded?

@mosra mosra marked this pull request as ready for review October 29, 2021 12:10
@mosra
Copy link
Collaborator Author

mosra commented Oct 29, 2021

Ok! 🔥

@mosra mosra merged commit 0699485 into main Oct 29, 2021
@mosra mosra deleted the update-magnum10 branch October 29, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants