From 6d1f6cad344087032dc3e8a5402d2668022c142d Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Thu, 5 Sep 2024 17:47:08 -0400 Subject: [PATCH] Package Kit: Allocate attribute handlers with BumpAllocator. On my development VM, there were over 300,000 calls to malloc() from EntryAttributeHandler::HandleAttribute() alone, which had the most out of any AttributeHandler, but the others were still significant (over another 10,000 at least.) On systems with more packages and more attributes, there would be of course more calls to malloc(). Since the Handlers are allocated and freed in a "stack"-like configuration, we can use a simple "bump" allocation strategy with the AttributeHandlerContext to avoid calling malloc() at all. In my testing, the most memory that was used appeared to be around 2 KB or so (and the smallest was 216 bytes), so a single slab should suffice for this. AttributeHandlerContext seems to be created/destroyed around 530 times during the boot process on my test machine; allocating and freeing the allocator's slab page that many times should be negligible (allocations that large still go through the block allocator.) Performance-wise, the total time we spend with AttributeHandlerContext objects "alive" goes from around ~172ms to ~156ms. So, not as much an improvement as one might hope, but that just goes to show that our kernel malloc() is pretty efficient. And this change will also keep short-lived objects off the heap during a period when we are allocating many long-lived objects, anyway. Change-Id: I810888434aad788511f2af30143335009b34ee78 Reviewed-on: https://review.haiku-os.org/c/haiku/+/8230 Tested-by: Commit checker robot Reviewed-by: waddlesplash --- headers/private/package/hpkg/ReaderImplBase.h | 10 ++++ src/kits/package/hpkg/PackageReaderImpl.cpp | 8 +-- src/kits/package/hpkg/ReaderImplBase.cpp | 55 +++++++++++++++---- .../package/hpkg/RepositoryReaderImpl.cpp | 2 +- 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/headers/private/package/hpkg/ReaderImplBase.h b/headers/private/package/hpkg/ReaderImplBase.h index 57f386a2cb..898918bdc1 100644 --- a/headers/private/package/hpkg/ReaderImplBase.h +++ b/headers/private/package/hpkg/ReaderImplBase.h @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -219,6 +220,8 @@ public: BHPKGPackageSectionID section; + BumpAllocator<> handlersAllocator; + public: AttributeHandlerContext( BErrorOutput* errorOutput, @@ -232,6 +235,7 @@ public: lowLevelHandler, BHPKGPackageSectionID section, bool ignoreUnknownAttributes); + ~AttributeHandlerContext(); void ErrorOccurred(); }; @@ -240,6 +244,9 @@ public: class ReaderImplBase::AttributeHandler : public SinglyLinkedListLinkImpl { public: + void* operator new(size_t size, AttributeHandlerContext* context); + void operator delete(void* pointer); + virtual ~AttributeHandler(); void SetLevel(int level); @@ -254,6 +261,9 @@ public: protected: int fLevel; + +private: + bool fDeleting; }; diff --git a/src/kits/package/hpkg/PackageReaderImpl.cpp b/src/kits/package/hpkg/PackageReaderImpl.cpp index ac04ccd53c..4b307fdbf0 100644 --- a/src/kits/package/hpkg/PackageReaderImpl.cpp +++ b/src/kits/package/hpkg/PackageReaderImpl.cpp @@ -91,7 +91,7 @@ struct PackageReaderImpl::AttributeAttributeHandler : AttributeHandler { status_t error = context->packageContentHandler->HandleEntryAttribute( fEntry, &fAttribute); - delete this; + AttributeHandler::Delete(context); return error; } @@ -127,7 +127,7 @@ struct PackageReaderImpl::EntryAttributeHandler : AttributeHandler { } // create handler - EntryAttributeHandler* handler = new(std::nothrow) + EntryAttributeHandler* handler = new(context) EntryAttributeHandler(context, parentEntry, name); if (handler == NULL) return B_NO_MEMORY; @@ -197,7 +197,7 @@ struct PackageReaderImpl::EntryAttributeHandler : AttributeHandler { return error; if (_handler != NULL) { - *_handler = new(std::nothrow) AttributeAttributeHandler( + *_handler = new(context) AttributeAttributeHandler( &fEntry, value.string); if (*_handler == NULL) return B_NO_MEMORY; @@ -232,7 +232,7 @@ struct PackageReaderImpl::EntryAttributeHandler : AttributeHandler { else context->packageContentHandler->HandleEntryDone(&fEntry); - delete this; + AttributeHandler::Delete(context); return error; } diff --git a/src/kits/package/hpkg/ReaderImplBase.cpp b/src/kits/package/hpkg/ReaderImplBase.cpp index a054c74667..016cb0ee0b 100644 --- a/src/kits/package/hpkg/ReaderImplBase.cpp +++ b/src/kits/package/hpkg/ReaderImplBase.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -69,6 +70,11 @@ ReaderImplBase::AttributeHandlerContext::AttributeHandlerContext( } +ReaderImplBase::AttributeHandlerContext::~AttributeHandlerContext() +{ +} + + void ReaderImplBase::AttributeHandlerContext::ErrorOccurred() { @@ -111,10 +117,37 @@ ReaderImplBase::AttributeHandler::NotifyDone( } +// #pragma mark - AttributeHandler allocation + + +void* +ReaderImplBase::AttributeHandler::operator new(size_t size, AttributeHandlerContext* context) +{ + AttributeHandler* handler = (AttributeHandler*)context->handlersAllocator.Allocate(size); + if (handler != NULL) + handler->fDeleting = false; + return handler; +} + + +void +ReaderImplBase::AttributeHandler::operator delete(void* pointer) +{ + AttributeHandler* handler = (AttributeHandler*)pointer; + if (!handler->fDeleting) + debugger("Package AttributeHandler: deleted without calling Delete()"); + + // Nothing else to do; memory is released by Delete(). +} + + status_t ReaderImplBase::AttributeHandler::Delete(AttributeHandlerContext* context) { + fDeleting = true; delete this; + + context->handlersAllocator.Free(this); return B_OK; } @@ -229,7 +262,7 @@ ReaderImplBase::PackageResolvableAttributeHandler::HandleAttribute( fPackageInfoValue.resolvable.version.major = value.string; if (_handler != NULL) { *_handler - = new(std::nothrow) PackageVersionAttributeHandler( + = new(context) PackageVersionAttributeHandler( fPackageInfoValue, fPackageInfoValue.resolvable.version, false); if (*_handler == NULL) @@ -242,7 +275,7 @@ ReaderImplBase::PackageResolvableAttributeHandler::HandleAttribute( fPackageInfoValue.resolvable.compatibleVersion.major = value.string; if (_handler != NULL) { *_handler - = new(std::nothrow) PackageVersionAttributeHandler( + = new(context) PackageVersionAttributeHandler( fPackageInfoValue, fPackageInfoValue.resolvable.compatibleVersion, false); if (*_handler == NULL) @@ -300,7 +333,7 @@ ReaderImplBase::PackageResolvableExpressionAttributeHandler::HandleAttribute( = value.string; if (_handler != NULL) { *_handler - = new(std::nothrow) PackageVersionAttributeHandler( + = new(context) PackageVersionAttributeHandler( fPackageInfoValue, fPackageInfoValue.resolvableExpression.version, false); @@ -536,7 +569,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( fPackageInfoValue.version.major = value.string; if (_handler != NULL) { *_handler - = new(std::nothrow) PackageVersionAttributeHandler( + = new(context) PackageVersionAttributeHandler( fPackageInfoValue, fPackageInfoValue.version, true); if (*_handler == NULL) return B_NO_MEMORY; @@ -566,7 +599,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( fPackageInfoValue.attributeID = B_PACKAGE_INFO_PROVIDES; if (_handler != NULL) { *_handler - = new(std::nothrow) PackageResolvableAttributeHandler( + = new(context) PackageResolvableAttributeHandler( fPackageInfoValue); if (*_handler == NULL) return B_NO_MEMORY; @@ -598,7 +631,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( break; } if (_handler != NULL) { - *_handler = new(std::nothrow) + *_handler = new(context) PackageResolvableExpressionAttributeHandler( fPackageInfoValue); if (*_handler == NULL) @@ -626,7 +659,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( = B_PACKAGE_INFO_GLOBAL_WRITABLE_FILES; if (_handler != NULL) { *_handler - = new(std::nothrow) GlobalWritableFileInfoAttributeHandler( + = new(context) GlobalWritableFileInfoAttributeHandler( fPackageInfoValue); if (*_handler == NULL) return B_NO_MEMORY; @@ -639,7 +672,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( = B_PACKAGE_INFO_USER_SETTINGS_FILES; if (_handler != NULL) { *_handler - = new(std::nothrow) UserSettingsFileInfoAttributeHandler( + = new(context) UserSettingsFileInfoAttributeHandler( fPackageInfoValue); if (*_handler == NULL) return B_NO_MEMORY; @@ -650,7 +683,7 @@ ReaderImplBase::PackageAttributeHandler::HandleAttribute( fPackageInfoValue.user.name = value.string; fPackageInfoValue.attributeID = B_PACKAGE_INFO_USERS; if (_handler != NULL) { - *_handler = new(std::nothrow) UserAttributeHandler( + *_handler = new(context) UserAttributeHandler( fPackageInfoValue); if (*_handler == NULL) return B_NO_MEMORY; @@ -734,7 +767,7 @@ ReaderImplBase::LowLevelAttributeHandler::HandleAttribute( // create a subhandler for the attribute, if it has children if (_handler != NULL) { - *_handler = new(std::nothrow) LowLevelAttributeHandler(id, value, + *_handler = new(context) LowLevelAttributeHandler(id, value, fToken, token); if (*_handler == NULL) { context->lowLevelHandler->HandleAttributeDone((BHPKGAttributeID)id, @@ -1129,7 +1162,7 @@ ReaderImplBase::_ParseAttributeTree(AttributeHandlerContext* context) if (hasChildren) { // create an ignore handler, if necessary if (childHandler == NULL) { - childHandler = new(std::nothrow) IgnoreAttributeHandler; + childHandler = new(context) IgnoreAttributeHandler; if (childHandler == NULL) { fErrorOutput->PrintError("Error: Out of memory!\n"); return B_NO_MEMORY; diff --git a/src/kits/package/hpkg/RepositoryReaderImpl.cpp b/src/kits/package/hpkg/RepositoryReaderImpl.cpp index 362ec7a419..b097a35806 100644 --- a/src/kits/package/hpkg/RepositoryReaderImpl.cpp +++ b/src/kits/package/hpkg/RepositoryReaderImpl.cpp @@ -74,7 +74,7 @@ public: return error; } - *_handler = new(std::nothrow) PackageAttributeHandler; + *_handler = new(context) PackageAttributeHandler; if (*_handler == NULL) return B_NO_MEMORY;