facebookresearch / habitat-sim

A flexible, high-performance 3D simulator for Embodied AI research.
https://aihabitat.org/
MIT License
2.48k stars 406 forks source link

--ConfigValue Updates - Matrix4 support, pointer-backed data store. Rev 2.0 #2369

Closed jturner65 closed 3 months ago

jturner65 commented 3 months ago

Motivation and Context

This is an updated, corrected version of an earlier PR which got merged before some leaks it was causing were caught and it was subsequently reverted. Mea culpa.

From Original PR :

This PR changes how esp::core::config::Configurations store data by having the underlying ConfigValue class store a pointer to the data in question instead of directly storing the data (i.e. the class member _data is now a pointer to a pointer to the data instead of a pointer to the data) if the data type is too large to be stored within the _data buffer. This PR also introduces, as a way to demonstrate this new feature, support for a new type in the configs, Magnum::Matrix4.

Currently on main branch, every ConfigValue includes a buffer that holds the actual data being consumed; due to the size of this buffer, every ConfigValue takes up 72 bytes of memory. This PR decreases this to 16 bytes + whatever heap allocation is necessary to hold the larger-than-8 byte types (the config value consists of a data array, now 8 bytes, and an enum denoting the type, 4 bytes, along with 4 bytes padding.)

Here's the memory footprint for each ConfigValue, along with any heap allocation if necessary (for larger objects), in this PR.

**Type        New**
ConfigValue buffer stores value : 
bool          16
int           16
double        16
Vector2       16
Mn::Rad       16
ConfigValue buffer stores ptr to value : 
Vector3      16 + 12
Vector4      16 + 16   (same for quaternion)
Matrix 3     16 + 36
String       16 + 32
Matrix4 (incoming in this PR) 16 + 64

How Has This Been Tested

C++ and python tests pass

Valgrind on viewer.cpp for leak detection

Types of changes

Checklist