diff options
author | Xisco Fauli <xiscofauli@libreoffice.org> | 2023-02-06 16:05:05 +0100 |
---|---|---|
committer | Noel Grandin <noel.grandin@collabora.co.uk> | 2023-02-06 21:46:40 +0000 |
commit | 05b2bfc289df8712097cc1e640bf7d3bc6b86a84 (patch) | |
tree | 176bfea3e856153b568a91f5fd2eab888e4723ef /comphelper | |
parent | 36aa8b43daccff63ab9b2f2c3df064abd2517a28 (diff) |
Revert "optimize ConfigurationProperty::get()"
This reverts commit 7df433cdc33b4d6ba38eafad9282d015571433ef
and its follow-ups:
* b4b63d0c46979ad6b6aae5d6a4ea6672ea248e10 "try to fix some shutdown
crashes"
* 203ad037ccb9fdebffea4f622229ded90635eb8b "try to fix shutdown crashes in
ConfigurationWrapper"
since it introduced a crash starting with LibreOffice 7.4
See https://crashreport.libreoffice.org/stats/signature/void%20rtl::str::release%3C_rtl_uString%3E(_rtl_uString*)
Later, it was reverted in libreoffice-7-4 branch with
df79a29ea20fb698d650be45a48c607f54476dea "Revert "optimize
ConfigurationProperty::get()" (7.4 only)" so the crash
is no longer reported in that branch.
OTOH, Noel tried to fix it in master/libreoffice-7-5 branches
with the two commits mentioned above but the crash is still being
reported in LibreOffice 7.5
After talking to him, he suggested to revert it all
Finally, adapt code to make loplugin:stringviewparam happy
with getPropertyValue
Change-Id: Id9fa97d2a81a590e53abd027e978d2d6179222b8
Change-Id: Id2e3475c342770be5705a74b9c0b45f45d6be5ad
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/146586
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Diffstat (limited to 'comphelper')
-rw-r--r-- | comphelper/source/misc/configuration.cxx | 105 |
1 files changed, 28 insertions, 77 deletions
diff --git a/comphelper/source/misc/configuration.cxx b/comphelper/source/misc/configuration.cxx index b84f705f1db0..59631dbccd83 100644 --- a/comphelper/source/misc/configuration.cxx +++ b/comphelper/source/misc/configuration.cxx @@ -10,8 +10,11 @@ #include <sal/config.h> #include <cassert> +#include <map> +#include <memory> +#include <mutex> +#include <string_view> -#include <com/sun/star/beans/NamedValue.hpp> #include <com/sun/star/beans/PropertyAttribute.hpp> #include <com/sun/star/configuration/ReadOnlyAccess.hpp> #include <com/sun/star/configuration/ReadWriteAccess.hpp> @@ -21,16 +24,12 @@ #include <com/sun/star/container/XHierarchicalNameReplace.hpp> #include <com/sun/star/container/XNameAccess.hpp> #include <com/sun/star/container/XNameContainer.hpp> -#include <com/sun/star/util/XChangesListener.hpp> -#include <com/sun/star/util/XChangesNotifier.hpp> -#include <com/sun/star/lang/DisposedException.hpp> #include <com/sun/star/lang/XLocalizable.hpp> #include <com/sun/star/uno/Any.hxx> #include <com/sun/star/uno/Reference.hxx> #include <comphelper/solarmutex.hxx> #include <comphelper/configuration.hxx> #include <comphelper/configurationlistener.hxx> -#include <cppuhelper/implbase.hxx> #include <rtl/ustring.hxx> #include <sal/log.hxx> #include <i18nlangtag/languagetag.hxx> @@ -107,65 +106,12 @@ comphelper::detail::ConfigurationWrapper::get() return WRAPPER; } -class comphelper::detail::ConfigurationChangesListener - : public ::cppu::WeakImplHelper<css::util::XChangesListener> -{ - comphelper::detail::ConfigurationWrapper& mrConfigurationWrapper; -public: - ConfigurationChangesListener(comphelper::detail::ConfigurationWrapper& rWrapper) - : mrConfigurationWrapper(rWrapper) - {} - // util::XChangesListener - virtual void SAL_CALL changesOccurred( const css::util::ChangesEvent& ) override - { - std::scoped_lock aGuard(mrConfigurationWrapper.maMutex); - mrConfigurationWrapper.maPropertyCache.clear(); - } - virtual void SAL_CALL disposing(const css::lang::EventObject&) override - { - std::scoped_lock aGuard(mrConfigurationWrapper.maMutex); - mrConfigurationWrapper.mbDisposed = true; - mrConfigurationWrapper.maPropertyCache.clear(); - } -}; - comphelper::detail::ConfigurationWrapper::ConfigurationWrapper(): context_(comphelper::getProcessComponentContext()), - access_(css::configuration::ReadWriteAccess::create(context_, "*")), - mbDisposed(false) -{ - // Set up a configuration notifier to invalidate the cache as needed. - try - { - css::uno::Reference< css::lang::XMultiServiceFactory > xConfigProvider( - css::configuration::theDefaultProvider::get( context_ ) ); - - // set root path - css::uno::Sequence< css::uno::Any > params { - css::uno::Any( css::beans::NamedValue{ "nodepath", css::uno::Any( OUString("/"))} ), - css::uno::Any( css::beans::NamedValue{ "locale", css::uno::Any( OUString("*"))} ) }; - - css::uno::Reference< css::uno::XInterface > xCfg - = xConfigProvider->createInstanceWithArguments(u"com.sun.star.configuration.ConfigurationAccess", - params); - - maNotifier = css::uno::Reference< css::util::XChangesNotifier >(xCfg, css::uno::UNO_QUERY); - assert(maNotifier.is()); - maListener = css::uno::Reference< ConfigurationChangesListener >(new ConfigurationChangesListener(*this)); - maNotifier->addChangesListener(maListener); - } - catch(const css::uno::Exception&) - { - assert(false); - } -} + access_(css::configuration::ReadWriteAccess::create(context_, "*")) +{} -comphelper::detail::ConfigurationWrapper::~ConfigurationWrapper() -{ - maPropertyCache.clear(); - maNotifier.clear(); - maListener.clear(); -} +comphelper::detail::ConfigurationWrapper::~ConfigurationWrapper() {} bool comphelper::detail::ConfigurationWrapper::isReadOnly(OUString const & path) const @@ -176,26 +122,31 @@ bool comphelper::detail::ConfigurationWrapper::isReadOnly(OUString const & path) != 0; } -css::uno::Any comphelper::detail::ConfigurationWrapper::getPropertyValue(OUString const& path) const +css::uno::Any comphelper::detail::ConfigurationWrapper::getPropertyValue(std::u16string_view path) const { - std::scoped_lock aGuard(maMutex); - if (mbDisposed) - throw css::lang::DisposedException(); // Cache the configuration access, since some of the keys are used in hot code. - auto it = maPropertyCache.find(path); - if( it != maPropertyCache.end()) - return it->second; + // Note that this cache is only used by the officecfg:: auto-generated code, using it for anything + // else would be unwise because the cache could end up containing stale entries. + static std::mutex gMutex; + static std::map<OUString, css::uno::Reference< css::container::XNameAccess >> gAccessMap; - sal_Int32 idx = path.lastIndexOf("/"); + sal_Int32 idx = path.rfind('/'); assert(idx!=-1); - OUString parentPath = path.copy(0, idx); - OUString childName = path.copy(idx+1); - - css::uno::Reference<css::container::XNameAccess> access( - access_->getByHierarchicalName(parentPath), css::uno::UNO_QUERY_THROW); - css::uno::Any property = access->getByName(childName); - maPropertyCache.emplace(path, property); - return property; + OUString parentPath(path.substr(0, idx)); + OUString childName(path.substr(idx+1)); + + std::scoped_lock aGuard(gMutex); + + // check cache + auto it = gAccessMap.find(parentPath); + if (it == gAccessMap.end()) + { + // not in the cache, look it up + css::uno::Reference<css::container::XNameAccess> access( + access_->getByHierarchicalName(parentPath), css::uno::UNO_QUERY_THROW); + it = gAccessMap.emplace(parentPath, access).first; + } + return it->second->getByName(childName); } void comphelper::detail::ConfigurationWrapper::setPropertyValue( |