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

Small allocation fix in fbx loader #4494

Merged
merged 10 commits into from May 16, 2023

Conversation

FlorianBorn71
Copy link
Contributor

This PR addresses a problem with loading very large binary fbx files. We had a case where the parsing code performed one billion(!) single small allocation which eventually crashed on any system.
This PR introduces class StackAllocator which can be used to allocate many small objects in a deque pattern. After parsing, the single objects are not de-allocated individually but instead the whole allocator is discarded.
This fix not only allows for loading much larger fbx files (in our case ~16GB with 3 million individual parts) but also improves parsing speed significantly on all other medium to large fbx files.

@turol
Copy link
Member

turol commented Apr 20, 2022

First, StackAllocator should be made noncopyable to avoid mishaps. Second, is this actually safe? It never runs destructors and the allocated objects are not trivial. LazyObject contains a std::unique_ptr and Connection contains a std::string. This sounds like a memory leak to me.

@FlorianBorn71
Copy link
Contributor Author

Thanks Turo, indeed the current version leaks memory - I will have to re-iterate

@FlorianBorn71
Copy link
Contributor Author

proper deallocation handled in new commit

Copy link
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

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

The stack-allocator looks great. I would recommend not to include a cpp-file. If you need this for linkage just define this in an inline file and include this in the header.

code/CMakeLists.txt Show resolved Hide resolved
@FlorianBorn71
Copy link
Contributor Author

FlorianBorn71 commented Apr 21, 2022

Hi @kimkulling, the compilation issues above do not make too much sense, it looks like it's compiling with a cached old .h file.
Is there a way to trigger a clean build?
[EDIT] nm, I think it was a merge issue, it did not take my modified version of the file. Fixing...

#define new_LazyObject new (allocator.Allocate(sizeof(LazyObject))) LazyObject
#define new_Connection new (allocator.Allocate(sizeof(Connection))) Connection
#define delete_LazyObject(_p) (_p)->~LazyObject()
#define delete_Connection(_p) (_p)->~Connection()
Copy link
Contributor

Choose a reason for hiding this comment

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

Object construction and destruction should be delegated to the function related to the allocator. Generally it would be better not to use macross like that overall.

// #include <new>

public:
    template<class T, class... Args>
    T* ConstructObject(Args&&... args)
    {
        return ::new(this->Allocate(sizeof(T))) T(std::forward<Args>(args)...);
    }

    template<class T>
    void DestroyObject(T* obj) noexcept
    {
        obj->~T();
    }

code/Common/StackAllocator.h Show resolved Hide resolved
FreeAll();
}

inline void *StackAllocator::Allocate(size_t byteSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function returns unaligned memory. It should take in additional argument called alignment and align memory internaly.

StackAllocator sa;

char* c = new (sa.Allocate(sizeof(char))) char(1);
std::string* s = new (sa.Allocate(sizeof(std::string))) std::string("str"); // This is UB?
std::cout << reinterpret_cast<size_t>(s) % alignof(std::string) << '\n';

@kimkulling
Copy link
Member

Will get fixed by #5096

@kimkulling kimkulling merged commit e627f69 into assimp:master May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants