comphelper/source/misc/configuration.cxx | 108 ++++++++----------------------- include/comphelper/configuration.hxx | 22 +----- 2 files changed, 32 insertions(+), 98 deletions(-)
New commits: commit cfcd6b25575087366924e39cf352bf3bb3853d6a Author: Xisco Fauli <xiscofa...@libreoffice.org> AuthorDate: Thu Feb 8 17:10:03 2024 +0100 Commit: Xisco Fauli <xiscofa...@libreoffice.org> CommitDate: Thu Feb 8 21:33:15 2024 +0100 tdf#157042: Revert "re-apply "optimize ConfigurationProperty::get()"" This reverts commit 3a4a00a51acca8f9b5e775547abff0c4dc9144d7. <x1sc0> noelgrandin, regarding tdf#157042, the commit was reverted in 24-2 and 7-6 branches but not in master so I was wondering what to do next. There are clear steps on how to reproduce it in comment 27 but it seems the crash is not reproducible with a debug build ( according to comment 37 ) <noelgrandin> x1sc0, let me try to reproduce that <noelgrandin> x1sc0, I cant reproduce, please just revert that on master Change-Id: I45dcf8f4b422e1a19eaa41ec7614db569b5aac7c Reviewed-on: https://gerrit.libreoffice.org/c/core/+/163125 Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk> Tested-by: Jenkins Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org> diff --git a/comphelper/source/misc/configuration.cxx b/comphelper/source/misc/configuration.cxx index ef89867dd50b..b7d97bbb6533 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> @@ -109,68 +108,13 @@ 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(); - mrConfigurationWrapper.maNotifier.clear(); - mrConfigurationWrapper.maListener.clear(); - } -}; - comphelper::detail::ConfigurationWrapper::ConfigurationWrapper( css::uno::Reference<css::uno::XComponentContext> const & context): context_(context.is() ? 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"_ustr, - params); - - maNotifier = css::uno::Reference< css::util::XChangesNotifier >(xCfg, css::uno::UNO_QUERY); - assert(maNotifier.is()); - maListener.set(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 @@ -181,30 +125,34 @@ 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(); - // should be short-circuited in ConfigurationProperty::get() assert(!comphelper::IsFuzzing()); // 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( diff --git a/include/comphelper/configuration.hxx b/include/comphelper/configuration.hxx index f248decad3a5..ede1af3c68be 100644 --- a/include/comphelper/configuration.hxx +++ b/include/comphelper/configuration.hxx @@ -12,16 +12,15 @@ #include <sal/config.h> +#include <optional> +#include <string_view> + #include <com/sun/star/uno/Any.hxx> #include <com/sun/star/uno/Reference.h> #include <comphelper/comphelperdllapi.h> #include <comphelper/processfactory.hxx> #include <sal/types.h> #include <memory> -#include <mutex> -#include <optional> -#include <string_view> -#include <unordered_map> namespace com::sun::star { namespace configuration { class XReadWriteAccess; } @@ -32,10 +31,6 @@ namespace com::sun::star { class XNameContainer; } namespace uno { class XComponentContext; } - namespace util { - class XChangesListener; - class XChangesNotifier; - } } namespace comphelper { @@ -87,18 +82,15 @@ private: namespace detail { -class ConfigurationChangesListener; - /// @internal class COMPHELPER_DLLPUBLIC ConfigurationWrapper { -friend class ConfigurationChangesListener; public: static ConfigurationWrapper const & get( css::uno::Reference<css::uno::XComponentContext> const & context); bool isReadOnly(OUString const & path) const; - css::uno::Any getPropertyValue(OUString const & path) const; + css::uno::Any getPropertyValue(std::u16string_view path) const; static void setPropertyValue( std::shared_ptr< ConfigurationChanges > const & batch, @@ -147,12 +139,6 @@ private: // css.beans.XHierarchicalPropertySetInfo), but then // configmgr::Access::asProperty() would report all properties as // READONLY, so isReadOnly() would not work - - mutable std::mutex maMutex; - bool mbDisposed; - mutable std::unordered_map<OUString, css::uno::Any> maPropertyCache; - css::uno::Reference< css::util::XChangesNotifier > maNotifier; - css::uno::Reference< css::util::XChangesListener > maListener; }; /// @internal