fix(physics): jolt static vectors being freed#1418
Conversation
Bound methods that return a vector by value (`Normalized()`, `AddRVec3()`) do not allocate. Emscripten's binding writes into one fixed static per method and hands back that address, so the result is only valid until the next call to the same method, and `JOLT.destroy` on one runs `free()` on an address `malloc` never issued. `createAnchorPoint` returned `jointOrigin.AddRVec3(jointOriginOffset)` straight through, so every caller was handed a static. `PhysicsSystem` then freed it in four places: two bare `JOLT.destroy(anchorPoint)` calls on the ball constraint path, and two `convertJoltRVec3ToJoltVec3(anchorPoint)` calls whose `destroy` parameter defaults to true. Each one asked dlmalloc to reclaim a data segment address it has no record of. Add `ownVec3`/`ownRVec3` and use them so no static escapes the expression that produced it. `createAnchorPoint` and `getPerpendicular` now return heap vectors the caller owns, which is what makes those frees legal.
Settings structs declare their vector members by value, so assigning into one memcpys the bytes in and the struct never takes ownership. Destroying the settings does not free the source vector, which means every `new JOLT.Vec3` and `convert*` result assigned into settings is still ours. Several were dropped instead: `wheelPos`, the four `urdfWheelBasis` vectors from `inferURDFAutoWheelBasis`, and `minBounds`/`maxBounds`. These run at joint and body creation rather than per frame, so the leak is bounded by spawns and is small in absolute terms, but it is unreclaimed all the same. Destroying `anchorPoint` is only valid because `createAnchorPoint` now returns a real allocation.
Ownership depends on where a vector came from, and that is not visible in any signature. These tests make the two cases concrete: `new JOLT.Vec3` is a real `malloc` that hands out a fresh address each time and recycles it on destroy, while a bound method's return value is one fixed static that sits below malloc's block, survives 2000 calls without accumulating, and is clobbered by the next call. Also covers the struct-copies-the-bytes-in rule that the destroys in `PhysicsSystem` depend on, and checks that `getPerpendicular` copies out of the static so callers get distinct heap vectors.
|
Also could you not write your pr descriptions with claude please, it's just like a really annoying style |
|
Also, please use the pr description template in the future |
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
* fix(constraints): vector cleanup Signed-off-by: Dhruv Arora <a_dhruv@outlook.com> * fix(jolt): removing vector destroy & fixing comment Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com> Co-authored-by: Azalea Colburn <azalea.colburn@autodesk.com> --------- Signed-off-by: Dhruv Arora <a_dhruv@outlook.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
BrandonPacewic
left a comment
There was a problem hiding this comment.
I have mixed feelings about this pr. While it is true that we were not aware of this behavior there is no bug associated with these changes. (Specific to the static alias function return behaviour of course.)
I believe that #1434 will fix all the other memory changes addressed in this pr so those are not needed here (#1434 addresses a much larger scope of issues which is why I'm saying that it should take priority of fixing the other issues listed in this pr).
Adding a small comment to the top of this particular function (createAnchorPoint) seems warranted, generally do not want the other changes presented here imo.
With that being said #1434 does add this small comment addressing this side effect so I'm unsure of what to do with this pull request in that case:
| export function createAnchorPoint(jointInstance: mirabuf.joint.JointInstance, jointDefinition: mirabuf.joint.Joint) { | ||
| const jointOrigin = jointDefinition.origin | ||
| const anchorPoint = jointDefinition.origin | ||
| ? convertMirabufVector3ToJoltRVec3(jointDefinition.origin) | ||
| : new JOLT.RVec3(0, 0, 0) | ||
| // TODO: Offset transformation for robot builder. | ||
| const jointOriginOffset = jointInstance.offset | ||
| ? convertMirabufVector3ToJoltRVec3(jointInstance.offset) | ||
| : new JOLT.RVec3(0, 0, 0) | ||
| ? convertMirabufVector3ToJoltVec3(jointInstance.offset) | ||
| : new JOLT.Vec3(0, 0, 0) | ||
|
|
||
| const anchorPoint = jointOrigin.AddRVec3(jointOriginOffset) | ||
| anchorPoint.Add(jointOriginOffset) |
There was a problem hiding this comment.
Note: this change from returning a Jolt.RVec3 to a Jolt.Vec3 is not in #1434
Task
Preventing memory corruption bug.
Symptom
Jolt.Vec3().Normalize()stores the result in a static temp memory allocation for all vectors. This means that if you dothen the value of a is overridden by the value of b.
this also is the case with the functions
AddRVec3()&Add()Solution
Copying the values into a new Jolt.Vec3() through the new
ownVec3()function.This, however, requires that the return value of functions such as
getPerpendicular()must be destroyedinstead of
-> this version leaks the vector from getPerpendicular
More information below and test file to show concept above in more depth:
Details
Testing file:
JoltOwnership.test.ts
Task
Jolt.Vec3 is stored in heap but
anything a bound method returns by value (
Normalized(),AddRVec3(),Add()) is not allocated. the binding keeps one static per method.How it looks:
So, we instead will copy the values from this memory and store it in a heap allocated Jolt.Vec3 for safer memory management. This means, however, that these Jolt.Vec3 s will need to be Jolt.destroy(x) ed.
Symptom
createAnchorPointreturnedjointOrigin.AddRVec3(...)straight through, so callers got a static temp.PhysicsSystemfreed it in four places: twoJOLT.destroy(anchorPoint)on the ball constraint path, and twoconvertJoltRVec3ToJoltVec3(anchorPoint)whosedestroyparam defaults to true.the function
JOLT.getPointer()shows thatAddRVec3returns 54992 every call, malloc's block (heap) starts around 1157600. all four were freeing a data segment address (meaning they did nothing).In addition
settings members are declared by value (
RVec3 mPoint1;), so setters memcpy and the struct never takes ownership.JOLT.destroy(settings)won't free a vector you assigned in, but you can destroy it yourself right after assigning. we were droppingwheelPos, the foururdfWheelBasisvectors, andminBounds/maxBounds.Solution
ownVec3/ownRVec3helpers so no static escapes the expression that made it.createAnchorPointandgetPerpendicularnow return owned heap vectorsanchorPointnow that it's real memoryJoltOwnership.test.tspins down both halves: fresh address per malloc, recycling on destroy, the static below the heap, 2000 calls yielding one address, the clobber, and structs copying bytes intesting
JoltOwnership.test.tspasses in chromium + firefox