From 1d6ed840fb9382c3234b719bbb97f72c836ac4d2 Mon Sep 17 00:00:00 2001 From: Florian Born Date: Wed, 20 Apr 2022 12:14:35 +0200 Subject: [PATCH 1/8] Replace single allocations in fbx loader by block allocation --- code/Common/StackAllocator.cpp | 84 ++++++++++++++++++++++++++++++++ code/Common/StackAllocator.h | 89 ++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) create mode 100644 code/Common/StackAllocator.cpp create mode 100644 code/Common/StackAllocator.h diff --git a/code/Common/StackAllocator.cpp b/code/Common/StackAllocator.cpp new file mode 100644 index 000000000..c789099a2 --- /dev/null +++ b/code/Common/StackAllocator.cpp @@ -0,0 +1,84 @@ +/* +Open Asset Import Library (assimp) +---------------------------------------------------------------------- + +Copyright (c) 2006-2022, assimp team + + +All rights reserved. + +Redistribution and use of this software in source and binary forms, +with or without modification, are permitted provided that the +following conditions are met: + +* Redistributions of source code must retain the above + copyright notice, this list of conditions and the + following disclaimer. + +* Redistributions in binary form must reproduce the above + copyright notice, this list of conditions and the + following disclaimer in the documentation and/or other + materials provided with the distribution. + +* Neither the name of the assimp team, nor the names of its + contributors may be used to endorse or promote products + derived from this software without specific prior + written permission of the assimp team. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +---------------------------------------------------------------------- +*/ + +#include "StackAllocator.h" +#include + +using namespace Assimp; + + +StackAllocator::StackAllocator() { +} + +StackAllocator::~StackAllocator() { + FreeAll(); +} + +void *StackAllocator::Allocate(size_t byteSize) { + if (m_subIndex + byteSize > m_blockAllocationSize) // start a new block + { + // double block size every time, up to maximum of g_maxBytesPerBlock. + // Block size must be at least as large as byteSize, but we want to use this for small allocations anyway. + m_blockAllocationSize = std::max(std::min(m_blockAllocationSize * 2, g_maxBytesPerBlock), byteSize); + uint8_t *data = (uint8_t *)malloc(m_blockAllocationSize); + m_storageBlocks.push_back(data); + m_subIndex = byteSize; + return data; + } + + uint8_t *data = m_storageBlocks.back(); + data += m_subIndex; + m_subIndex += byteSize; + + return data; +} + +void StackAllocator::FreeAll() { + for (size_t i = 0; i < m_storageBlocks.size(); i++) { + free(m_storageBlocks[i]); + } + std::deque empty; + m_storageBlocks.swap(empty); + // start over: + m_blockAllocationSize = g_startBytesPerBlock; + m_subIndex = g_maxBytesPerBlock; +} diff --git a/code/Common/StackAllocator.h b/code/Common/StackAllocator.h new file mode 100644 index 000000000..b0f1ecfe1 --- /dev/null +++ b/code/Common/StackAllocator.h @@ -0,0 +1,89 @@ +/* +Open Asset Import Library (assimp) +---------------------------------------------------------------------- + +Copyright (c) 2006-2022, assimp team + + +All rights reserved. + +Redistribution and use of this software in source and binary forms, +with or without modification, are permitted provided that the +following conditions are met: + +* Redistributions of source code must retain the above + copyright notice, this list of conditions and the + following disclaimer. + +* Redistributions in binary form must reproduce the above + copyright notice, this list of conditions and the + following disclaimer in the documentation and/or other + materials provided with the distribution. + +* Neither the name of the assimp team, nor the names of its + contributors may be used to endorse or promote products + derived from this software without specific prior + written permission of the assimp team. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +---------------------------------------------------------------------- +*/ + +/** @file StackAllocator.h + * @brief A very bare-bone allocator class that is suitable when + * allocating many small objects, e.g. during parsing. + * Individual objects are not freed, instead only the whole memory + * can be deallocated. + */ +#ifndef AI_STACK_ALLOCATOR_H_INC +#define AI_STACK_ALLOCATOR_H_INC + + +#include + +namespace Assimp +{ + +/** @brief A very bare-bone allocator class that is suitable when + * allocating many small objects, e.g. during parsing. + * Individual objects are not freed, instead only the whole memory + * can be deallocated. +*/ +class StackAllocator { +public: + // Constructs the allocator + StackAllocator(); + // Destructs the allocator and frees all memory + ~StackAllocator(); + + // Returns a pointer to byteSize bytes of heap memory that persists + // for the lifetime of the allocator (or until FreeAll is called). + void *Allocate(size_t byteSize); + + // Releases all the memory owned by this allocator. + // Memory provided through function Allocate is not valid anymore after this function has been called. + void FreeAll(); + +private: + constexpr const static size_t g_maxBytesPerBlock = 64 * 1024 * 1024; // The maximum size (in bytes) of a block + constexpr const static size_t g_startBytesPerBlock = 16 * 1024; // Size of the first block. Next blocks will double in size until maximum size of g_maxBytesPerBlock + size_t m_blockAllocationSize = g_startBytesPerBlock; // Block size of the current block + size_t m_subIndex = g_maxBytesPerBlock; // The current byte offset in the current block + std::deque m_storageBlocks; // A list of blocks +}; + + +} // namespace Assimp + +#endif // include guard From 2b3c49cb93d404a023fe8b852ebdb3e331fa6a15 Mon Sep 17 00:00:00 2001 From: Florian Born Date: Wed, 20 Apr 2022 12:33:39 +0200 Subject: [PATCH 2/8] All allocation changes --- code/AssetLib/FBX/FBXBinaryTokenizer.cpp | 10 ++++------ code/AssetLib/FBX/FBXDocument.cpp | 23 ++++++++++------------- code/AssetLib/FBX/FBXDocument.h | 2 ++ code/AssetLib/FBX/FBXImporter.cpp | 10 ++++------ code/AssetLib/FBX/FBXParser.cpp | 20 ++++++++------------ code/AssetLib/FBX/FBXParser.h | 17 +++++++++++------ code/AssetLib/FBX/FBXTokenizer.cpp | 20 ++++++++++---------- code/AssetLib/FBX/FBXTokenizer.h | 7 ++++--- code/CMakeLists.txt | 2 ++ 9 files changed, 55 insertions(+), 56 deletions(-) diff --git a/code/AssetLib/FBX/FBXBinaryTokenizer.cpp b/code/AssetLib/FBX/FBXBinaryTokenizer.cpp index 1a4d11856..43154e2ab 100644 --- a/code/AssetLib/FBX/FBXBinaryTokenizer.cpp +++ b/code/AssetLib/FBX/FBXBinaryTokenizer.cpp @@ -341,8 +341,7 @@ void ReadData(const char*& sbegin_out, const char*& send_out, const char* input, // ------------------------------------------------------------------------------------------------ -bool ReadScope(TokenList& output_tokens, const char* input, const char*& cursor, const char* end, bool const is64bits) -{ +bool ReadScope(TokenList &output_tokens, StackAllocator &token_allocator, const char *input, const char *&cursor, const char *end, bool const is64bits) { // the first word contains the offset at which this block ends const uint64_t end_offset = is64bits ? ReadDoubleWord(input, cursor, end) : ReadWord(input, cursor, end); @@ -408,7 +407,7 @@ bool ReadScope(TokenList& output_tokens, const char* input, const char*& cursor, // XXX this is vulnerable to stack overflowing .. while(Offset(input, cursor) < end_offset - sentinel_block_length) { - ReadScope(output_tokens, input, cursor, input + end_offset - sentinel_block_length, is64bits); + ReadScope(output_tokens, token_allocator, input, cursor, input + end_offset - sentinel_block_length, is64bits); } output_tokens.push_back(new_Token(cursor, cursor + 1, TokenType_CLOSE_BRACKET, Offset(input, cursor) )); @@ -431,8 +430,7 @@ bool ReadScope(TokenList& output_tokens, const char* input, const char*& cursor, // ------------------------------------------------------------------------------------------------ // TODO: Test FBX Binary files newer than the 7500 version to check if the 64 bits address behaviour is consistent -void TokenizeBinary(TokenList& output_tokens, const char* input, size_t length) -{ +void TokenizeBinary(TokenList &output_tokens, const char *input, size_t length, StackAllocator &token_allocator) { ai_assert(input); ASSIMP_LOG_DEBUG("Tokenizing binary FBX file"); @@ -465,7 +463,7 @@ void TokenizeBinary(TokenList& output_tokens, const char* input, size_t length) try { while (cursor < end ) { - if (!ReadScope(output_tokens, input, cursor, input + length, is64bits)) { + if (!ReadScope(output_tokens, token_allocator, input, cursor, input + length, is64bits)) { break; } } diff --git a/code/AssetLib/FBX/FBXDocument.cpp b/code/AssetLib/FBX/FBXDocument.cpp index b49ee625a..17b347f86 100644 --- a/code/AssetLib/FBX/FBXDocument.cpp +++ b/code/AssetLib/FBX/FBXDocument.cpp @@ -235,7 +235,7 @@ FileGlobalSettings::FileGlobalSettings(const Document &doc, std::shared_ptrCompound(); for(const ElementMap::value_type& el : sobjects.Elements()) { @@ -377,7 +372,7 @@ void Document::ReadObjects() { DOMWarning("encountered duplicate object id, ignoring first occurrence",el.second); } - objects[id] = new LazyObject(id, *el.second, *this); + objects[id] = new_LazyObject(id, *el.second, *this); // grab all animation stacks upfront since there is no listing of them if(!strcmp(el.first.c_str(),"AnimationStack")) { @@ -444,8 +439,10 @@ void Document::ReadPropertyTemplates() { } // ------------------------------------------------------------------------------------------------ -void Document::ReadConnections() { - const Scope& sc = parser.GetRootScope(); +void Document::ReadConnections() +{ + StackAllocator &allocator = parser.GetAllocator(); + const Scope &sc = parser.GetRootScope(); // read property templates from "Definitions" section const Element* const econns = sc["Connections"]; if(!econns || !econns->Compound()) { @@ -484,7 +481,7 @@ void Document::ReadConnections() { } // add new connection - const Connection* const c = new Connection(insertionOrder++,src,dest,prop,*this); + const Connection* const c = new_Connection(insertionOrder++,src,dest,prop,*this); src_connections.insert(ConnectionMap::value_type(src,c)); dest_connections.insert(ConnectionMap::value_type(dest,c)); } diff --git a/code/AssetLib/FBX/FBXDocument.h b/code/AssetLib/FBX/FBXDocument.h index c61a47410..142e870ff 100644 --- a/code/AssetLib/FBX/FBXDocument.h +++ b/code/AssetLib/FBX/FBXDocument.h @@ -80,6 +80,8 @@ class BlendShape; class Skin; class Cluster; +#define new_LazyObject new (allocator.Allocate(sizeof(LazyObject))) LazyObject +#define new_Connection new (allocator.Allocate(sizeof(Connection))) Connection /** Represents a delay-parsed FBX objects. Many objects in the scene * are not needed by assimp, so it makes no sense to parse them diff --git a/code/AssetLib/FBX/FBXImporter.cpp b/code/AssetLib/FBX/FBXImporter.cpp index 0f63acc8f..96c5a9f14 100644 --- a/code/AssetLib/FBX/FBXImporter.cpp +++ b/code/AssetLib/FBX/FBXImporter.cpp @@ -160,17 +160,18 @@ void FBXImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy TokenList tokens; try { + Assimp::StackAllocator tempAllocator; bool is_binary = false; if (!strncmp(begin, "Kaydara FBX Binary", 18)) { is_binary = true; - TokenizeBinary(tokens, begin, contents.size()); + TokenizeBinary(tokens, begin, contents.size(), tempAllocator); } else { - Tokenize(tokens, begin); + Tokenize(tokens, begin, tempAllocator); } // use this information to construct a very rudimentary // parse-tree representing the FBX scope structure - Parser parser(tokens, is_binary); + Parser parser(tokens, tempAllocator, is_binary); // take the raw parse-tree and convert it to a FBX DOM Document doc(parser, settings); @@ -189,10 +190,7 @@ void FBXImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy // Set FBX file scale is relative to CM must be converted to M for // assimp universal format (M) SetFileScale(size_relative_to_cm * 0.01f); - - std::for_each(tokens.begin(), tokens.end(), Util::delete_fun()); } catch (std::exception &) { - std::for_each(tokens.begin(), tokens.end(), Util::delete_fun()); throw; } } diff --git a/code/AssetLib/FBX/FBXParser.cpp b/code/AssetLib/FBX/FBXParser.cpp index e20377a3c..e6cc39f46 100644 --- a/code/AssetLib/FBX/FBXParser.cpp +++ b/code/AssetLib/FBX/FBXParser.cpp @@ -117,6 +117,7 @@ namespace FBX { // ------------------------------------------------------------------------------------------------ Element::Element(const Token& key_token, Parser& parser) : key_token(key_token) { TokenPtr n = nullptr; + StackAllocator &allocator = parser.GetAllocator(); do { n = parser.AdvanceToNextToken(); if(!n) { @@ -145,7 +146,7 @@ Element::Element(const Token& key_token, Parser& parser) : key_token(key_token) } if (n->Type() == TokenType_OPEN_BRACKET) { - compound.reset(new Scope(parser)); + compound.reset(new_Scope(parser)); // current token should be a TOK_CLOSE_BRACKET n = parser.CurrentToken(); @@ -178,6 +179,7 @@ Scope::Scope(Parser& parser,bool topLevel) } } + StackAllocator &allocator = parser.GetAllocator(); TokenPtr n = parser.AdvanceToNextToken(); if (n == nullptr) { ParseError("unexpected end of file"); @@ -208,22 +210,16 @@ Scope::Scope(Parser& parser,bool topLevel) } // ------------------------------------------------------------------------------------------------ -Scope::~Scope() { - for(ElementMap::value_type& v : elements) { - delete v.second; - } +Scope::~Scope() +{ } // ------------------------------------------------------------------------------------------------ -Parser::Parser (const TokenList& tokens, bool is_binary) -: tokens(tokens) -, last() -, current() -, cursor(tokens.begin()) -, is_binary(is_binary) +Parser::Parser(const TokenList &tokens, StackAllocator &allocator, bool is_binary) : + tokens(tokens), allocator(allocator), last(), current(), cursor(tokens.begin()), is_binary(is_binary) { ASSIMP_LOG_DEBUG("Parsing FBX tokens"); - root.reset(new Scope(*this,true)); + root = new_Scope(*this, true); } // ------------------------------------------------------------------------------------------------ diff --git a/code/AssetLib/FBX/FBXParser.h b/code/AssetLib/FBX/FBXParser.h index 314481e42..5eed61fef 100644 --- a/code/AssetLib/FBX/FBXParser.h +++ b/code/AssetLib/FBX/FBXParser.h @@ -52,6 +52,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include +#include "Common/StackAllocator.h" #include "FBXCompileConfig.h" #include "FBXTokenizer.h" @@ -68,8 +69,8 @@ typedef std::fbx_unordered_multimap< std::string, Element* > ElementMap; typedef std::pair ElementCollection; -# define new_Scope new Scope -# define new_Element new Element +#define new_Scope new (allocator.Allocate(sizeof(Scope))) Scope +#define new_Element new (allocator.Allocate(sizeof(Element))) Element /** FBX data entity that consists of a key:value tuple. @@ -159,17 +160,21 @@ class Parser public: /** Parse given a token list. Does not take ownership of the tokens - * the objects must persist during the entire parser lifetime */ - Parser (const TokenList& tokens,bool is_binary); + Parser(const TokenList &tokens, StackAllocator &allocator, bool is_binary); ~Parser(); const Scope& GetRootScope() const { - return *root.get(); + return *root; } bool IsBinary() const { return is_binary; } + StackAllocator &GetAllocator() { + return allocator; + } + private: friend class Scope; friend class Element; @@ -180,10 +185,10 @@ private: private: const TokenList& tokens; - + StackAllocator &allocator; TokenPtr last, current; TokenList::const_iterator cursor; - std::unique_ptr root; + Scope *root; const bool is_binary; }; diff --git a/code/AssetLib/FBX/FBXTokenizer.cpp b/code/AssetLib/FBX/FBXTokenizer.cpp index 8698abac6..7f03b71da 100644 --- a/code/AssetLib/FBX/FBXTokenizer.cpp +++ b/code/AssetLib/FBX/FBXTokenizer.cpp @@ -96,7 +96,8 @@ AI_WONT_RETURN void TokenizeError(const std::string& message, unsigned int line, // process a potential data token up to 'cur', adding it to 'output_tokens'. // ------------------------------------------------------------------------------------------------ -void ProcessDataToken( TokenList& output_tokens, const char*& start, const char*& end, +void ProcessDataToken(TokenList &output_tokens, StackAllocator &token_allocator, + const char*& start, const char*& end, unsigned int line, unsigned int column, TokenType type = TokenType_DATA, @@ -133,8 +134,7 @@ void ProcessDataToken( TokenList& output_tokens, const char*& start, const char* } // ------------------------------------------------------------------------------------------------ -void Tokenize(TokenList& output_tokens, const char* input) -{ +void Tokenize(TokenList &output_tokens, const char *input, StackAllocator &token_allocator) { ai_assert(input); ASSIMP_LOG_DEBUG("Tokenizing ASCII FBX file"); @@ -166,7 +166,7 @@ void Tokenize(TokenList& output_tokens, const char* input) in_double_quotes = false; token_end = cur; - ProcessDataToken(output_tokens,token_begin,token_end,line,column); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column); pending_data_token = false; } continue; @@ -183,30 +183,30 @@ void Tokenize(TokenList& output_tokens, const char* input) continue; case ';': - ProcessDataToken(output_tokens,token_begin,token_end,line,column); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column); comment = true; continue; case '{': - ProcessDataToken(output_tokens,token_begin,token_end, line, column); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column); output_tokens.push_back(new_Token(cur,cur+1,TokenType_OPEN_BRACKET,line,column)); continue; case '}': - ProcessDataToken(output_tokens,token_begin,token_end,line,column); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column); output_tokens.push_back(new_Token(cur,cur+1,TokenType_CLOSE_BRACKET,line,column)); continue; case ',': if (pending_data_token) { - ProcessDataToken(output_tokens,token_begin,token_end,line,column,TokenType_DATA,true); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column, TokenType_DATA, true); } output_tokens.push_back(new_Token(cur,cur+1,TokenType_COMMA,line,column)); continue; case ':': if (pending_data_token) { - ProcessDataToken(output_tokens,token_begin,token_end,line,column,TokenType_KEY,true); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column, TokenType_KEY, true); } else { TokenizeError("unexpected colon", line, column); @@ -228,7 +228,7 @@ void Tokenize(TokenList& output_tokens, const char* input) } } - ProcessDataToken(output_tokens,token_begin,token_end,line,column,type); + ProcessDataToken(output_tokens, token_allocator, token_begin, token_end, line, column, type); } pending_data_token = false; diff --git a/code/AssetLib/FBX/FBXTokenizer.h b/code/AssetLib/FBX/FBXTokenizer.h index 877950945..d5f7999e4 100644 --- a/code/AssetLib/FBX/FBXTokenizer.h +++ b/code/AssetLib/FBX/FBXTokenizer.h @@ -47,6 +47,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #define INCLUDED_AI_FBX_TOKENIZER_H #include "FBXCompileConfig.h" +#include "Common/StackAllocator.h" #include #include #include @@ -158,7 +159,7 @@ private: typedef const Token* TokenPtr; typedef std::vector< TokenPtr > TokenList; -#define new_Token new Token +#define new_Token new (token_allocator.Allocate(sizeof(Token))) Token /** Main FBX tokenizer function. Transform input buffer into a list of preprocessed tokens. @@ -168,7 +169,7 @@ typedef std::vector< TokenPtr > TokenList; * @param output_tokens Receives a list of all tokens in the input data. * @param input_buffer Textual input buffer to be processed, 0-terminated. * @throw DeadlyImportError if something goes wrong */ -void Tokenize(TokenList& output_tokens, const char* input); +void Tokenize(TokenList &output_tokens, const char *input, StackAllocator &tokenAllocator); /** Tokenizer function for binary FBX files. @@ -179,7 +180,7 @@ void Tokenize(TokenList& output_tokens, const char* input); * @param input_buffer Binary input buffer to be processed. * @param length Length of input buffer, in bytes. There is no 0-terminal. * @throw DeadlyImportError if something goes wrong */ -void TokenizeBinary(TokenList& output_tokens, const char* input, size_t length); +void TokenizeBinary(TokenList &output_tokens, const char *input, size_t length, StackAllocator &tokenAllocator); } // ! FBX diff --git a/code/CMakeLists.txt b/code/CMakeLists.txt index 5b08c0f37..e7eaf1d84 100644 --- a/code/CMakeLists.txt +++ b/code/CMakeLists.txt @@ -193,6 +193,8 @@ SET( Common_SRCS Common/ScenePreprocessor.cpp Common/ScenePreprocessor.h Common/SkeletonMeshBuilder.cpp + Common/StackAllocator.h + Common/StackAllocator.cpp Common/StandardShapes.cpp Common/TargetAnimation.cpp Common/TargetAnimation.h From d3646c3118b9b93fa7fe5dc9a979424450f8f72a Mon Sep 17 00:00:00 2001 From: Florian Born Date: Wed, 20 Apr 2022 16:11:09 +0200 Subject: [PATCH 3/8] Proper destruction of individual objects --- code/AssetLib/FBX/FBXDocument.cpp | 13 ++++++++++++- code/AssetLib/FBX/FBXDocument.h | 2 ++ code/AssetLib/FBX/FBXImporter.cpp | 14 +++++++++----- code/AssetLib/FBX/FBXParser.cpp | 17 ++++++++++++++--- code/AssetLib/FBX/FBXParser.h | 7 ++++--- code/AssetLib/FBX/FBXTokenizer.h | 1 + code/AssetLib/FBX/FBXUtil.h | 11 +++++++++++ code/Common/StackAllocator.h | 4 ++++ 8 files changed, 57 insertions(+), 12 deletions(-) diff --git a/code/AssetLib/FBX/FBXDocument.cpp b/code/AssetLib/FBX/FBXDocument.cpp index 17b347f86..437bf210b 100644 --- a/code/AssetLib/FBX/FBXDocument.cpp +++ b/code/AssetLib/FBX/FBXDocument.cpp @@ -257,7 +257,18 @@ Document::Document(Parser& parser, const ImportSettings& settings) : } // ------------------------------------------------------------------------------------------------ -Document::~Document() { +Document::~Document() +{ + // The document does not own the memory for the following objects, but we need to call their d'tor + // so they can properly free memory like string members: + + for (ObjectMap::value_type &v : objects) { + delete_LazyObject(v.second); + } + + for (ConnectionMap::value_type &v : src_connections) { + delete_Connection(v.second); + } // |dest_connections| contain the same Connection objects as the |src_connections| } diff --git a/code/AssetLib/FBX/FBXDocument.h b/code/AssetLib/FBX/FBXDocument.h index 142e870ff..11ee17f4c 100644 --- a/code/AssetLib/FBX/FBXDocument.h +++ b/code/AssetLib/FBX/FBXDocument.h @@ -82,6 +82,8 @@ class Cluster; #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() /** Represents a delay-parsed FBX objects. Many objects in the scene * are not needed by assimp, so it makes no sense to parse them diff --git a/code/AssetLib/FBX/FBXImporter.cpp b/code/AssetLib/FBX/FBXImporter.cpp index 96c5a9f14..fa2122c4f 100644 --- a/code/AssetLib/FBX/FBXImporter.cpp +++ b/code/AssetLib/FBX/FBXImporter.cpp @@ -158,9 +158,8 @@ void FBXImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy // broadphase tokenizing pass in which we identify the core // syntax elements of FBX (brackets, commas, key:value mappings) TokenList tokens; - try { - - Assimp::StackAllocator tempAllocator; + Assimp::StackAllocator tempAllocator; + try { bool is_binary = false; if (!strncmp(begin, "Kaydara FBX Binary", 18)) { is_binary = true; @@ -190,8 +189,13 @@ void FBXImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy // Set FBX file scale is relative to CM must be converted to M for // assimp universal format (M) SetFileScale(size_relative_to_cm * 0.01f); - } catch (std::exception &) { - throw; + + // This collection does not own the memory for the tokens, but we need to call their d'tor + std::for_each(tokens.begin(), tokens.end(), Util::destructor_fun()); + + } catch (std::exception &) { + std::for_each(tokens.begin(), tokens.end(), Util::destructor_fun()); + throw; } } diff --git a/code/AssetLib/FBX/FBXParser.cpp b/code/AssetLib/FBX/FBXParser.cpp index e6cc39f46..976cdb90b 100644 --- a/code/AssetLib/FBX/FBXParser.cpp +++ b/code/AssetLib/FBX/FBXParser.cpp @@ -115,7 +115,9 @@ namespace Assimp { namespace FBX { // ------------------------------------------------------------------------------------------------ -Element::Element(const Token& key_token, Parser& parser) : key_token(key_token) { +Element::Element(const Token& key_token, Parser& parser) : + key_token(key_token), compound(nullptr) +{ TokenPtr n = nullptr; StackAllocator &allocator = parser.GetAllocator(); do { @@ -146,7 +148,7 @@ Element::Element(const Token& key_token, Parser& parser) : key_token(key_token) } if (n->Type() == TokenType_OPEN_BRACKET) { - compound.reset(new_Scope(parser)); + compound = new_Scope(parser); // current token should be a TOK_CLOSE_BRACKET n = parser.CurrentToken(); @@ -166,6 +168,10 @@ Element::Element(const Token& key_token, Parser& parser) : key_token(key_token) // ------------------------------------------------------------------------------------------------ Element::~Element() { + if (compound) { + delete_Scope(compound); + } + // no need to delete tokens, they are owned by the parser } @@ -212,6 +218,11 @@ Scope::Scope(Parser& parser,bool topLevel) // ------------------------------------------------------------------------------------------------ Scope::~Scope() { + // This collection does not own the memory for the elements, but we need to call their d'tor: + + for (ElementMap::value_type &v : elements) { + delete_Element(v.second); + } } // ------------------------------------------------------------------------------------------------ @@ -225,7 +236,7 @@ Parser::Parser(const TokenList &tokens, StackAllocator &allocator, bool is_binar // ------------------------------------------------------------------------------------------------ Parser::~Parser() { - // empty + delete_Scope(root); } // ------------------------------------------------------------------------------------------------ diff --git a/code/AssetLib/FBX/FBXParser.h b/code/AssetLib/FBX/FBXParser.h index 5eed61fef..9b654239b 100644 --- a/code/AssetLib/FBX/FBXParser.h +++ b/code/AssetLib/FBX/FBXParser.h @@ -71,7 +71,8 @@ typedef std::pair Element #define new_Scope new (allocator.Allocate(sizeof(Scope))) Scope #define new_Element new (allocator.Allocate(sizeof(Element))) Element - +#define delete_Scope(_p) (_p)->~Scope() +#define delete_Element(_p) (_p)->~Element() /** FBX data entity that consists of a key:value tuple. * @@ -91,7 +92,7 @@ public: ~Element(); const Scope* Compound() const { - return compound.get(); + return compound; } const Token& KeyToken() const { @@ -105,7 +106,7 @@ public: private: const Token& key_token; TokenList tokens; - std::unique_ptr compound; + Scope* compound; }; /** FBX data entity that consists of a 'scope', a collection diff --git a/code/AssetLib/FBX/FBXTokenizer.h b/code/AssetLib/FBX/FBXTokenizer.h index d5f7999e4..7e395d5e9 100644 --- a/code/AssetLib/FBX/FBXTokenizer.h +++ b/code/AssetLib/FBX/FBXTokenizer.h @@ -160,6 +160,7 @@ typedef const Token* TokenPtr; typedef std::vector< TokenPtr > TokenList; #define new_Token new (token_allocator.Allocate(sizeof(Token))) Token +#define delete_Token(_p) (_p)->~Token() /** Main FBX tokenizer function. Transform input buffer into a list of preprocessed tokens. diff --git a/code/AssetLib/FBX/FBXUtil.h b/code/AssetLib/FBX/FBXUtil.h index 0e0bb75be..4674a5054 100644 --- a/code/AssetLib/FBX/FBXUtil.h +++ b/code/AssetLib/FBX/FBXUtil.h @@ -66,6 +66,17 @@ struct delete_fun } }; +/** helper for std::for_each to call the destructor on all items in a container without freeing their heap*/ +template +struct destructor_fun { + void operator()(const volatile T* del) { + if (del) { + del->~T(); + } + } +}; + + /** Get a string representation for a #TokenType. */ const char* TokenTypeString(TokenType t); diff --git a/code/Common/StackAllocator.h b/code/Common/StackAllocator.h index b0f1ecfe1..b670b7434 100644 --- a/code/Common/StackAllocator.h +++ b/code/Common/StackAllocator.h @@ -67,6 +67,10 @@ public: // Destructs the allocator and frees all memory ~StackAllocator(); + // non copyable + StackAllocator(const StackAllocator &) = delete; + StackAllocator &operator=(const StackAllocator &) = delete; + // Returns a pointer to byteSize bytes of heap memory that persists // for the lifetime of the allocator (or until FreeAll is called). void *Allocate(size_t byteSize); From 320775b9392cd809b663440839b9124da844518c Mon Sep 17 00:00:00 2001 From: Florian Born Date: Wed, 20 Apr 2022 16:39:36 +0200 Subject: [PATCH 4/8] Compile fix --- code/Common/StackAllocator.h | 1 + 1 file changed, 1 insertion(+) diff --git a/code/Common/StackAllocator.h b/code/Common/StackAllocator.h index b670b7434..cc252f8df 100644 --- a/code/Common/StackAllocator.h +++ b/code/Common/StackAllocator.h @@ -51,6 +51,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include +#include namespace Assimp { From 0355ae967feefd5811ddf4c37d91e43fff880a13 Mon Sep 17 00:00:00 2001 From: Florian Born Date: Wed, 20 Apr 2022 17:57:03 +0200 Subject: [PATCH 5/8] compile fix on other platforms --- code/Common/StackAllocator.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/Common/StackAllocator.cpp b/code/Common/StackAllocator.cpp index c789099a2..259a565be 100644 --- a/code/Common/StackAllocator.cpp +++ b/code/Common/StackAllocator.cpp @@ -59,7 +59,7 @@ void *StackAllocator::Allocate(size_t byteSize) { // double block size every time, up to maximum of g_maxBytesPerBlock. // Block size must be at least as large as byteSize, but we want to use this for small allocations anyway. m_blockAllocationSize = std::max(std::min(m_blockAllocationSize * 2, g_maxBytesPerBlock), byteSize); - uint8_t *data = (uint8_t *)malloc(m_blockAllocationSize); + uint8_t *data = new uint8_t[m_blockAllocationSize]; m_storageBlocks.push_back(data); m_subIndex = byteSize; return data; @@ -74,7 +74,7 @@ void *StackAllocator::Allocate(size_t byteSize) { void StackAllocator::FreeAll() { for (size_t i = 0; i < m_storageBlocks.size(); i++) { - free(m_storageBlocks[i]); + delete [] m_storageBlocks[i]; } std::deque empty; m_storageBlocks.swap(empty); From 7f0509ae87b852216ca853d7b7094fdb1cecf627 Mon Sep 17 00:00:00 2001 From: Florian Born Date: Thu, 21 Apr 2022 11:33:04 +0200 Subject: [PATCH 6/8] Stack allocator is now inline --- code/CMakeLists.txt | 2 +- code/Common/StackAllocator.h | 10 ++++++---- code/Common/{StackAllocator.cpp => StackAllocator.inl} | 8 ++++---- 3 files changed, 11 insertions(+), 9 deletions(-) rename code/Common/{StackAllocator.cpp => StackAllocator.inl} (93%) diff --git a/code/CMakeLists.txt b/code/CMakeLists.txt index e7eaf1d84..75a5cb377 100644 --- a/code/CMakeLists.txt +++ b/code/CMakeLists.txt @@ -194,7 +194,7 @@ SET( Common_SRCS Common/ScenePreprocessor.h Common/SkeletonMeshBuilder.cpp Common/StackAllocator.h - Common/StackAllocator.cpp + Common/StackAllocator.inl Common/StandardShapes.cpp Common/TargetAnimation.cpp Common/TargetAnimation.h diff --git a/code/Common/StackAllocator.h b/code/Common/StackAllocator.h index cc252f8df..aff36fa47 100644 --- a/code/Common/StackAllocator.h +++ b/code/Common/StackAllocator.h @@ -64,9 +64,9 @@ namespace Assimp class StackAllocator { public: // Constructs the allocator - StackAllocator(); + inline StackAllocator(); // Destructs the allocator and frees all memory - ~StackAllocator(); + inline ~StackAllocator(); // non copyable StackAllocator(const StackAllocator &) = delete; @@ -74,11 +74,11 @@ public: // Returns a pointer to byteSize bytes of heap memory that persists // for the lifetime of the allocator (or until FreeAll is called). - void *Allocate(size_t byteSize); + inline void *Allocate(size_t byteSize); // Releases all the memory owned by this allocator. // Memory provided through function Allocate is not valid anymore after this function has been called. - void FreeAll(); + inline void FreeAll(); private: constexpr const static size_t g_maxBytesPerBlock = 64 * 1024 * 1024; // The maximum size (in bytes) of a block @@ -91,4 +91,6 @@ private: } // namespace Assimp +#include "StackAllocator.inl" + #endif // include guard diff --git a/code/Common/StackAllocator.cpp b/code/Common/StackAllocator.inl similarity index 93% rename from code/Common/StackAllocator.cpp rename to code/Common/StackAllocator.inl index 259a565be..d973b7794 100644 --- a/code/Common/StackAllocator.cpp +++ b/code/Common/StackAllocator.inl @@ -46,14 +46,14 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. using namespace Assimp; -StackAllocator::StackAllocator() { +inline StackAllocator::StackAllocator() { } -StackAllocator::~StackAllocator() { +inline StackAllocator::~StackAllocator() { FreeAll(); } -void *StackAllocator::Allocate(size_t byteSize) { +inline void *StackAllocator::Allocate(size_t byteSize) { if (m_subIndex + byteSize > m_blockAllocationSize) // start a new block { // double block size every time, up to maximum of g_maxBytesPerBlock. @@ -72,7 +72,7 @@ void *StackAllocator::Allocate(size_t byteSize) { return data; } -void StackAllocator::FreeAll() { +inline void StackAllocator::FreeAll() { for (size_t i = 0; i < m_storageBlocks.size(); i++) { delete [] m_storageBlocks[i]; } From a415f33fb561d8e2531205aa61d1228e47158c19 Mon Sep 17 00:00:00 2001 From: Florian Born Date: Thu, 21 Apr 2022 16:09:28 +0200 Subject: [PATCH 7/8] merge failure, parts in this file were missing --- code/AssetLib/FBX/FBXDocument.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/FBX/FBXDocument.h b/code/AssetLib/FBX/FBXDocument.h index 11ee17f4c..c701056f6 100644 --- a/code/AssetLib/FBX/FBXDocument.h +++ b/code/AssetLib/FBX/FBXDocument.h @@ -1076,7 +1076,7 @@ private: /** DOM root for a FBX file */ class Document { public: - Document(const Parser& parser, const ImportSettings& settings); + Document(Parser& parser, const ImportSettings& settings); ~Document(); @@ -1160,7 +1160,7 @@ private: const ImportSettings& settings; ObjectMap objects; - const Parser& parser; + Parser& parser; PropertyTemplateMap templates; ConnectionMap src_connections; From b2ea018fd57326a6421375cc7999d08a7ee341a2 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 11 Sep 2022 17:04:58 +0200 Subject: [PATCH 8/8] Use user-define element destructor. --- code/AssetLib/FBX/FBXParser.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/AssetLib/FBX/FBXParser.h b/code/AssetLib/FBX/FBXParser.h index 597096548..67707342e 100644 --- a/code/AssetLib/FBX/FBXParser.h +++ b/code/AssetLib/FBX/FBXParser.h @@ -89,7 +89,7 @@ class Element { public: Element(const Token& key_token, Parser& parser); - ~Element() = default; + ~Element(): const Scope* Compound() const { return compound;