From b6ffedb03da628d809124da24b7f2c20252710a5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 10 Jul 2012 15:46:27 -0400 Subject: MAINT-1175: Reimplement LLTypeInfoLookup for better lookup failure. The original LLTypeInfoLookup implementation was based on two assumptions: small overall container size, and infrequent normal-case lookup failures. Those assumptions led to binary-searching a sorted vector, with linear search as a fallback to cover the problem case of two different type_info* values for the same type. As documented in the Jira, this turned out to be a problem. The container size was larger than expected, and failed lookups turned out to be far more common than expected. The new implementation is based on a hash map of std::type_info::name() strings, which should perform equally well in the success and failure cases: no special-case fallback logic. --- indra/llcommon/lltypeinfolookup.h | 119 +++++++++++++++++++------------------- 1 file changed, 61 insertions(+), 58 deletions(-) diff --git a/indra/llcommon/lltypeinfolookup.h b/indra/llcommon/lltypeinfolookup.h index 7510cc12ed..1e62d33488 100644 --- a/indra/llcommon/lltypeinfolookup.h +++ b/indra/llcommon/lltypeinfolookup.h @@ -12,8 +12,12 @@ #if ! defined(LL_LLTYPEINFOLOOKUP_H) #define LL_LLTYPEINFOLOOKUP_H -#include "llsortedvector.h" +#include +#include +#include +#include #include +#include /** * LLTypeInfoLookup is specifically designed for use cases for which you might @@ -22,40 +26,52 @@ * you can't rely on always getting the same std::type_info* for a given type: * different load modules will produce different std::type_info*. * LLTypeInfoLookup contains a workaround to address this issue. - * - * Specifically, when we don't find the passed std::type_info*, - * LLTypeInfoLookup performs a linear search over registered entries to - * compare name() strings. Presuming that this succeeds, we cache the new - * (previously unrecognized) std::type_info* to speed future lookups. - * - * This worst-case fallback search (linear search with string comparison) - * should only happen the first time we look up a given type from a particular - * load module other than the one from which we initially registered types. - * (However, a lookup which wouldn't succeed anyway will always have - * worst-case performance.) This class is probably best used with less than a - * few dozen different types. */ template class LLTypeInfoLookup { + // We present an interface like this: + typedef std::map intf_map_type; + // Use this for our underlying implementation: lookup by + // std::type_info::name() string. Note that we must store a std::pair -- in other words, an intf_map_type::value_type + // pair -- so we can present iterators that do the right thing. + // (This might result in a lookup with one std::type_info* returning an + // iterator to a different std::type_info*, but frankly, my dear, we don't + // give a damn.) + typedef boost::unordered_map impl_map_type; + // Iterator shorthand + typedef typename intf_map_type::iterator intf_iterator; + typedef typename intf_map_type::const_iterator intf_const_iterator; + typedef typename intf_map_type::value_type intf_value_type; + typedef typename impl_map_type::iterator impl_iterator; + typedef typename impl_map_type::const_iterator impl_const_iterator; + typedef typename impl_map_type::value_type impl_value_type; + // Type of function that transforms impl_value_type to intf_value_type + typedef boost::function iterfunc; + typedef boost::function const_iterfunc; + public: typedef LLTypeInfoLookup self; - typedef LLSortedVector vector_type; - typedef typename vector_type::key_type key_type; - typedef typename vector_type::mapped_type mapped_type; - typedef typename vector_type::value_type value_type; - typedef typename vector_type::iterator iterator; - typedef typename vector_type::const_iterator const_iterator; + typedef typename intf_map_type::key_type key_type; + typedef typename intf_map_type::mapped_type mapped_type; + typedef intf_value_type value_type; + + // Iterators are different because we have to transform + // impl_map_type::iterator to intf_map_type::iterator. + typedef boost::transform_iterator iterator; + typedef boost::transform_iterator const_iterator; LLTypeInfoLookup() {} - iterator begin() { return mVector.begin(); } - iterator end() { return mVector.end(); } - const_iterator begin() const { return mVector.begin(); } - const_iterator end() const { return mVector.end(); } - bool empty() const { return mVector.empty(); } - std::size_t size() const { return mVector.size(); } + iterator begin() { return transform(mMap.begin()); } + iterator end() { return transform(mMap.end()); } + const_iterator begin() const { return transform(mMap.begin()); } + const_iterator end() const { return transform(mMap.end()); } + bool empty() const { return mMap.empty(); } + std::size_t size() const { return mMap.size(); } + // Shorthand -- I've always wished std::map supported this signature. std::pair insert(const std::type_info* key, const VALUE& value) { return insert(value_type(key, value)); @@ -63,48 +79,35 @@ public: std::pair insert(const value_type& pair) { - return mVector.insert(pair); + // Obtain and store the std::type_info::name() string as the key. + // Save the whole incoming pair as the value! + std::pair + inserted(mMap.insert(impl_value_type(pair.first->name(), pair))); + // Have to transform the iterator before returning. + return std::pair(transform(inserted.first), inserted.second); } - // const find() forwards to non-const find(): this can alter mVector! - const_iterator find(const std::type_info* key) const + iterator find(const std::type_info* key) { - return const_cast(this)->find(key); + return transform(mMap.find(key->name())); } - // non-const find() caches previously-unknown type_info* to speed future - // lookups. - iterator find(const std::type_info* key) + const_iterator find(const std::type_info* key) const { - iterator found = mVector.find(key); - if (found != mVector.end()) - { - // If LLSortedVector::find() found, great, we're done. - return found; - } - // Here we didn't find the passed type_info*. On Linux, though, even - // for the same type, typeid(sametype) produces a different type_info* - // when used in different load modules. So the fact that we didn't - // find the type_info* we seek doesn't mean this type isn't - // registered. Scan for matching name() string. - for (typename vector_type::iterator ti(mVector.begin()), tend(mVector.end()); - ti != tend; ++ti) - { - if (std::string(ti->first->name()) == key->name()) - { - // This unrecognized 'key' is for the same type as ti->first. - // To speed future lookups, insert a new entry that lets us - // look up ti->second using this same 'key'. - return insert(key, ti->second).first; - } - } - // We simply have never seen a type with this type_info* from any load - // module. - return mVector.end(); + return transform(mMap.find(key->name())); } private: - vector_type mVector; + iterator transform(impl_iterator iter) + { + return boost::make_transform_iterator(iter, boost::mem_fn(&impl_value_type::second)); + } + const_iterator transform(impl_const_iterator iter) + { + return boost::make_transform_iterator(iter, boost::mem_fn(&impl_value_type::second)); + } + + impl_map_type mMap; }; #endif /* ! defined(LL_LLTYPEINFOLOOKUP_H) */ -- cgit v1.2.3 From 00ae56334c057c1ea5ad08c604b551fcbdf37a30 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 10 Jul 2012 16:41:26 -0400 Subject: MAINT-1175: Fix Windows build. It seems MSVC doesn't like boost::make_transform_iterator() in the context I was using it. Try directly invoking the iterator's constructor. --- indra/llcommon/lltypeinfolookup.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/lltypeinfolookup.h b/indra/llcommon/lltypeinfolookup.h index 1e62d33488..583ca8863b 100644 --- a/indra/llcommon/lltypeinfolookup.h +++ b/indra/llcommon/lltypeinfolookup.h @@ -100,11 +100,11 @@ public: private: iterator transform(impl_iterator iter) { - return boost::make_transform_iterator(iter, boost::mem_fn(&impl_value_type::second)); + return iterator(iter, boost::mem_fn(&impl_value_type::second)); } const_iterator transform(impl_const_iterator iter) { - return boost::make_transform_iterator(iter, boost::mem_fn(&impl_value_type::second)); + return const_iterator(iter, boost::mem_fn(&impl_value_type::second)); } impl_map_type mMap; -- cgit v1.2.3 From 70035274093e8803f8c7f28162feef311ef725b4 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 10 Jul 2012 17:29:58 -0400 Subject: MAINT-1175: Still grappling with MSVC idiosyncracies. Maybe it's failing to correctly handle overloaded transform() methods? --- indra/llcommon/lltypeinfolookup.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/indra/llcommon/lltypeinfolookup.h b/indra/llcommon/lltypeinfolookup.h index 583ca8863b..679cc51f1d 100644 --- a/indra/llcommon/lltypeinfolookup.h +++ b/indra/llcommon/lltypeinfolookup.h @@ -66,8 +66,8 @@ public: iterator begin() { return transform(mMap.begin()); } iterator end() { return transform(mMap.end()); } - const_iterator begin() const { return transform(mMap.begin()); } - const_iterator end() const { return transform(mMap.end()); } + const_iterator begin() const { return const_transform(mMap.begin()); } + const_iterator end() const { return const_transform(mMap.end()); } bool empty() const { return mMap.empty(); } std::size_t size() const { return mMap.size(); } @@ -94,7 +94,7 @@ public: const_iterator find(const std::type_info* key) const { - return transform(mMap.find(key->name())); + return const_transform(mMap.find(key->name())); } private: @@ -102,7 +102,7 @@ private: { return iterator(iter, boost::mem_fn(&impl_value_type::second)); } - const_iterator transform(impl_const_iterator iter) + const_iterator const_transform(impl_const_iterator iter) { return const_iterator(iter, boost::mem_fn(&impl_value_type::second)); } -- cgit v1.2.3 From 18bd525d00ee3ce16164900293ee6ea8c2204589 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Jul 2012 08:14:34 -0400 Subject: MAINT-1175: Forbid LLRegistry[Singleton]. Back out code that selects LLTypeInfoLookup for the underlying map implementation when KEY = [const] std::type_info*, because LLTypeInfoLookup's API is changing to become incompatible with std::map. Instead, fail with STATIC_ASSERT when LLRegistry's KEY is [const] std::type_info*. Fix all existing uses to use std::type_info::name() string instead. --- indra/llcommon/llregistry.h | 30 ++++++++++-------------------- indra/llui/llfloater.cpp | 2 +- indra/llui/llradiogroup.cpp | 2 +- indra/llui/llrngwriter.cpp | 2 +- indra/llui/llscrolllistcolumn.cpp | 2 +- indra/llui/lltextbase.cpp | 2 +- indra/llui/lltooltip.cpp | 2 +- indra/llui/lluictrlfactory.cpp | 6 +++--- indra/llui/lluictrlfactory.h | 4 ++-- indra/llui/llxuiparser.cpp | 2 +- indra/llui/llxuiparser.h | 3 ++- indra/newview/llavatarlistitem.cpp | 2 +- indra/newview/llinventorylistitem.cpp | 2 +- indra/newview/llwearableitemslist.cpp | 8 ++++---- 14 files changed, 30 insertions(+), 39 deletions(-) diff --git a/indra/llcommon/llregistry.h b/indra/llcommon/llregistry.h index 36d7f7a44c..843c169f3d 100644 --- a/indra/llcommon/llregistry.h +++ b/indra/llcommon/llregistry.h @@ -30,8 +30,8 @@ #include #include +#include #include "llsingleton.h" -#include "lltypeinfolookup.h" template class LLRegistryDefaultComparator @@ -39,27 +39,17 @@ class LLRegistryDefaultComparator bool operator()(const T& lhs, const T& rhs) { return lhs < rhs; } }; -template -struct LLRegistryMapSelector -{ - typedef std::map type; -}; - -template -struct LLRegistryMapSelector -{ - typedef LLTypeInfoLookup type; -}; - -template -struct LLRegistryMapSelector -{ - typedef LLTypeInfoLookup type; -}; - template > class LLRegistry { + // Do not use LLRegistry with KEY = std::type_info* or KEY = const std::type_info*. + // This is known to fail on Linux. + // If you must use LLRegistry with dynamic type info, use KEY = const char* + // and pass std::type_info::name(); this works across load modules. + // Disallow both std::type_info* and const std::type_info*. First remove + // the pointer, then remove const, then compare is_same. + BOOST_STATIC_ASSERT(! (boost::is_same::type>::type, std::type_info>::value)); + public: typedef LLRegistry registry_t; typedef typename boost::add_reference::type>::type ref_const_key_t; @@ -72,7 +62,7 @@ public: { friend class LLRegistry; public: - typedef typename LLRegistryMapSelector::type registry_map_t; + typedef std::map registry_map_t; bool add(ref_const_key_t key, ref_const_value_t value) { diff --git a/indra/llui/llfloater.cpp b/indra/llui/llfloater.cpp index 8ca1e685a9..f20cc2452f 100644 --- a/indra/llui/llfloater.cpp +++ b/indra/llui/llfloater.cpp @@ -232,7 +232,7 @@ void LLFloater::initClass() } // defaults for floater param block pulled from widgets/floater.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterFloaterParams(&typeid(LLFloater::Params), "floater"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterFloaterParams(typeid(LLFloater::Params).name(), "floater"); LLFloater::LLFloater(const LLSD& key, const LLFloater::Params& p) : LLPanel(), // intentionally do not pass params here, see initFromParams diff --git a/indra/llui/llradiogroup.cpp b/indra/llui/llradiogroup.cpp index 95a7d09382..71bbcd8ade 100644 --- a/indra/llui/llradiogroup.cpp +++ b/indra/llui/llradiogroup.cpp @@ -66,7 +66,7 @@ protected: LLSD mPayload; // stores data that this item represents in the radio group }; -static LLWidgetNameRegistry::StaticRegistrar register_radio_item(&typeid(LLRadioGroup::ItemParams), "radio_item"); +static LLWidgetNameRegistry::StaticRegistrar register_radio_item(typeid(LLRadioGroup::ItemParams).name(), "radio_item"); LLRadioGroup::Params::Params() : allow_deselect("allow_deselect"), diff --git a/indra/llui/llrngwriter.cpp b/indra/llui/llrngwriter.cpp index 5e6840d7df..90a1e77b1a 100644 --- a/indra/llui/llrngwriter.cpp +++ b/indra/llui/llrngwriter.cpp @@ -92,7 +92,7 @@ void LLRNGWriter::addDefinition(const std::string& type_name, const LLInitParam: // add includes for all possible children const std::type_info* type = *LLWidgetTypeRegistry::instance().getValue(type_name); - const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type); + const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type->name()); // add include declarations for all valid children for (widget_registry_t::Registrar::registry_map_t::const_iterator it = widget_registryp->currentRegistrar().beginItems(); diff --git a/indra/llui/llscrolllistcolumn.cpp b/indra/llui/llscrolllistcolumn.cpp index 07a6dfaa10..d132d0cc7e 100644 --- a/indra/llui/llscrolllistcolumn.cpp +++ b/indra/llui/llscrolllistcolumn.cpp @@ -39,7 +39,7 @@ const S32 MIN_COLUMN_WIDTH = 20; // defaults for LLScrollColumnHeader param block pulled from widgets/scroll_column_header.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterColumnHeaderParams(&typeid(LLScrollColumnHeader::Params), "scroll_column_header"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterColumnHeaderParams(typeid(LLScrollColumnHeader::Params).name(), "scroll_column_header"); //--------------------------------------------------------------------------- // LLScrollColumnHeader diff --git a/indra/llui/lltextbase.cpp b/indra/llui/lltextbase.cpp index 7aeeae298f..8314386632 100644 --- a/indra/llui/lltextbase.cpp +++ b/indra/llui/lltextbase.cpp @@ -131,7 +131,7 @@ struct LLTextBase::line_end_compare // // register LLTextBase::Params under name "textbase" -static LLWidgetNameRegistry::StaticRegistrar sRegisterTextBaseParams(&typeid(LLTextBase::Params), "textbase"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterTextBaseParams(typeid(LLTextBase::Params).name(), "textbase"); LLTextBase::LineSpacingParams::LineSpacingParams() : multiple("multiple", 1.f), diff --git a/indra/llui/lltooltip.cpp b/indra/llui/lltooltip.cpp index f737d48abf..0723dbec70 100644 --- a/indra/llui/lltooltip.cpp +++ b/indra/llui/lltooltip.cpp @@ -129,7 +129,7 @@ void LLToolTipView::drawStickyRect() } // defaults for floater param block pulled from widgets/floater.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterInspectorParams(&typeid(LLInspector::Params), "inspector"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterInspectorParams(typeid(LLInspector::Params).name(), "inspector"); // // LLToolTip diff --git a/indra/llui/lluictrlfactory.cpp b/indra/llui/lluictrlfactory.cpp index 25e7a31e90..fe3b470722 100644 --- a/indra/llui/lluictrlfactory.cpp +++ b/indra/llui/lluictrlfactory.cpp @@ -281,7 +281,7 @@ const LLInitParam::BaseBlock& get_empty_param_block() void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const std::type_info* param_block_type, const std::string& tag) { // associate parameter block type with template .xml file - std::string* existing_tag = LLWidgetNameRegistry::instance().getValue(param_block_type); + std::string* existing_tag = LLWidgetNameRegistry::instance().getValue(param_block_type->name()); if (existing_tag != NULL) { if(*existing_tag != tag) @@ -297,7 +297,7 @@ void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const st return; } } - LLWidgetNameRegistry::instance().defaultRegistrar().add(param_block_type, tag); + LLWidgetNameRegistry::instance().defaultRegistrar().add(param_block_type->name(), tag); //FIXME: comment this in when working on schema generation //LLWidgetTypeRegistry::instance().defaultRegistrar().add(tag, widget_type); //LLDefaultParamBlockRegistry::instance().defaultRegistrar().add(widget_type, &get_empty_param_block); @@ -306,6 +306,6 @@ void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const st //static const std::string* LLUICtrlFactory::getWidgetTag(const std::type_info* widget_type) { - return LLWidgetNameRegistry::instance().getValue(widget_type); + return LLWidgetNameRegistry::instance().getValue(widget_type->name()); } diff --git a/indra/llui/lluictrlfactory.h b/indra/llui/lluictrlfactory.h index d612ad5005..1f7a8e08ce 100644 --- a/indra/llui/lluictrlfactory.h +++ b/indra/llui/lluictrlfactory.h @@ -69,9 +69,9 @@ protected: friend class LLSingleton; }; -// lookup widget name by type +// lookup widget name by type (actually by std::type_info::name()) class LLWidgetNameRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; // lookup function for generating empty param block by widget type diff --git a/indra/llui/llxuiparser.cpp b/indra/llui/llxuiparser.cpp index afc76024d1..c3a246b9a5 100644 --- a/indra/llui/llxuiparser.cpp +++ b/indra/llui/llxuiparser.cpp @@ -606,7 +606,7 @@ void LLXUIXSDWriter::writeXSD(const std::string& type_name, const std::string& p // add includes for all possible children const std::type_info* type = *LLWidgetTypeRegistry::instance().getValue(type_name); - const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type); + const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type->name()); // add choices for valid children if (widget_registryp) diff --git a/indra/llui/llxuiparser.h b/indra/llui/llxuiparser.h index d7cd256967..3c092afdd6 100644 --- a/indra/llui/llxuiparser.h +++ b/indra/llui/llxuiparser.h @@ -55,8 +55,9 @@ typedef boost::function widget_registry_t; +// const char* key used for std::type_info::name() string class LLChildRegistryRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; diff --git a/indra/newview/llavatarlistitem.cpp b/indra/newview/llavatarlistitem.cpp index 30eecfe323..c07fec8e5e 100644 --- a/indra/newview/llavatarlistitem.cpp +++ b/indra/newview/llavatarlistitem.cpp @@ -44,7 +44,7 @@ S32 LLAvatarListItem::sLeftPadding = 0; S32 LLAvatarListItem::sNameRightPadding = 0; S32 LLAvatarListItem::sChildrenWidths[LLAvatarListItem::ALIC_COUNT]; -static LLWidgetNameRegistry::StaticRegistrar sRegisterAvatarListItemParams(&typeid(LLAvatarListItem::Params), "avatar_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterAvatarListItemParams(typeid(LLAvatarListItem::Params).name(), "avatar_list_item"); LLAvatarListItem::Params::Params() : default_style("default_style"), diff --git a/indra/newview/llinventorylistitem.cpp b/indra/newview/llinventorylistitem.cpp index 3e0849a795..5489671430 100644 --- a/indra/newview/llinventorylistitem.cpp +++ b/indra/newview/llinventorylistitem.cpp @@ -40,7 +40,7 @@ #include "llinventorymodel.h" #include "llviewerinventory.h" -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelInventoryListItemBaseParams(&typeid(LLPanelInventoryListItemBase::Params), "inventory_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelInventoryListItemBaseParams(typeid(LLPanelInventoryListItemBase::Params).name(), "inventory_list_item"); static const S32 WIDGET_SPACING = 3; diff --git a/indra/newview/llwearableitemslist.cpp b/indra/newview/llwearableitemslist.cpp index 92697fb2eb..abb53301a6 100644 --- a/indra/newview/llwearableitemslist.cpp +++ b/indra/newview/llwearableitemslist.cpp @@ -135,7 +135,7 @@ void LLPanelWearableOutfitItem::updateItem(const std::string& name, ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelClothingListItem(&typeid(LLPanelClothingListItem::Params), "clothing_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelClothingListItem(typeid(LLPanelClothingListItem::Params).name(), "clothing_list_item"); LLPanelClothingListItem::Params::Params() @@ -222,7 +222,7 @@ BOOL LLPanelClothingListItem::postBuild() ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelBodyPartsListItem(&typeid(LLPanelBodyPartsListItem::Params), "bodyparts_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelBodyPartsListItem(typeid(LLPanelBodyPartsListItem::Params).name(), "bodyparts_list_item"); LLPanelBodyPartsListItem::Params::Params() @@ -293,7 +293,7 @@ BOOL LLPanelBodyPartsListItem::postBuild() return TRUE; } -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDeletableWearableListItem(&typeid(LLPanelDeletableWearableListItem::Params), "deletable_wearable_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDeletableWearableListItem(typeid(LLPanelDeletableWearableListItem::Params).name(), "deletable_wearable_list_item"); LLPanelDeletableWearableListItem::Params::Params() : delete_btn("delete_btn") @@ -373,7 +373,7 @@ void LLPanelAttachmentListItem::updateItem(const std::string& name, ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDummyClothingListItem(&typeid(LLPanelDummyClothingListItem::Params), "dummy_clothing_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDummyClothingListItem(typeid(LLPanelDummyClothingListItem::Params).name(), "dummy_clothing_list_item"); LLPanelDummyClothingListItem::Params::Params() : add_panel("add_panel"), -- cgit v1.2.3 From 578d70dec0a01b5ed7b461c38503c082ac1a3608 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Jul 2012 08:20:14 -0400 Subject: MAINT-1175: Change LLTypeInfoLookup API for future optimizations. Per discussion with Richard, accept the type key for insert() and find() as a template parameter rather than as std::type_info*. This permits (e.g.) some sort of compile-time prehashing for common types, without changing the API. Eliminate iterators from the API altogether, thus avoiding costs associated with transform_iterator. Fix existing references in llinitparam.h. --- indra/llcommon/llinitparam.h | 24 +++++------ indra/llcommon/lltypeinfolookup.h | 90 +++++++++++---------------------------- 2 files changed, 38 insertions(+), 76 deletions(-) diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h index 99983a19cb..c0170e533b 100644 --- a/indra/llcommon/llinitparam.h +++ b/indra/llcommon/llinitparam.h @@ -242,20 +242,20 @@ namespace LLInitParam template bool readValue(T& param) { - parser_read_func_map_t::iterator found_it = mParserReadFuncs->find(&typeid(T)); - if (found_it != mParserReadFuncs->end()) + boost::optional found_it = mParserReadFuncs->find(); + if (found_it) { - return found_it->second(*this, (void*)¶m); + return (*found_it)(*this, (void*)¶m); } return false; } template bool writeValue(const T& param, name_stack_t& name_stack) { - parser_write_func_map_t::iterator found_it = mParserWriteFuncs->find(&typeid(T)); - if (found_it != mParserWriteFuncs->end()) + boost::optional found_it = mParserWriteFuncs->find(); + if (found_it) { - return found_it->second(*this, (const void*)¶m, name_stack); + return (*found_it)(*this, (const void*)¶m, name_stack); } return false; } @@ -263,10 +263,10 @@ namespace LLInitParam // dispatch inspection to registered inspection functions, for each parameter in a param block template bool inspectValue(name_stack_t& name_stack, S32 min_count, S32 max_count, const possible_values_t* possible_values) { - parser_inspect_func_map_t::iterator found_it = mParserInspectFuncs->find(&typeid(T)); - if (found_it != mParserInspectFuncs->end()) + boost::optional found_it = mParserInspectFuncs->find(); + if (found_it) { - found_it->second(name_stack, min_count, max_count, possible_values); + (*found_it)(name_stack, min_count, max_count, possible_values); return true; } return false; @@ -281,14 +281,14 @@ namespace LLInitParam template void registerParserFuncs(parser_read_func_t read_func, parser_write_func_t write_func = NULL) { - mParserReadFuncs->insert(std::make_pair(&typeid(T), read_func)); - mParserWriteFuncs->insert(std::make_pair(&typeid(T), write_func)); + mParserReadFuncs->insert(read_func); + mParserWriteFuncs->insert(write_func); } template void registerInspectFunc(parser_inspect_func_t inspect_func) { - mParserInspectFuncs->insert(std::make_pair(&typeid(T), inspect_func)); + mParserInspectFuncs->insert(inspect_func); } bool mParseSilently; diff --git a/indra/llcommon/lltypeinfolookup.h b/indra/llcommon/lltypeinfolookup.h index 679cc51f1d..5267e3d2fb 100644 --- a/indra/llcommon/lltypeinfolookup.h +++ b/indra/llcommon/lltypeinfolookup.h @@ -13,11 +13,8 @@ #define LL_LLTYPEINFOLOOKUP_H #include -#include -#include -#include +#include #include -#include /** * LLTypeInfoLookup is specifically designed for use cases for which you might @@ -26,87 +23,52 @@ * you can't rely on always getting the same std::type_info* for a given type: * different load modules will produce different std::type_info*. * LLTypeInfoLookup contains a workaround to address this issue. + * + * The API deliberately diverges from std::map in several respects: + * * It avoids iterators, not only begin()/end() but also as return values + * from insert() and find(). This bypasses transform_iterator overhead. + * * Since we literally use compile-time types as keys, the essential insert() + * and find() methods accept the key type as a @em template parameter, + * accepting and returning value_type as a normal runtime value. This is to + * permit future optimization (e.g. compile-time type hashing) without + * changing the API. */ template class LLTypeInfoLookup { - // We present an interface like this: - typedef std::map intf_map_type; // Use this for our underlying implementation: lookup by - // std::type_info::name() string. Note that we must store a std::pair -- in other words, an intf_map_type::value_type - // pair -- so we can present iterators that do the right thing. - // (This might result in a lookup with one std::type_info* returning an - // iterator to a different std::type_info*, but frankly, my dear, we don't - // give a damn.) - typedef boost::unordered_map impl_map_type; - // Iterator shorthand - typedef typename intf_map_type::iterator intf_iterator; - typedef typename intf_map_type::const_iterator intf_const_iterator; - typedef typename intf_map_type::value_type intf_value_type; - typedef typename impl_map_type::iterator impl_iterator; - typedef typename impl_map_type::const_iterator impl_const_iterator; - typedef typename impl_map_type::value_type impl_value_type; - // Type of function that transforms impl_value_type to intf_value_type - typedef boost::function iterfunc; - typedef boost::function const_iterfunc; + // std::type_info::name() string. This is one of the rare cases in which I + // dare use const char* directly, rather than std::string, because I'm + // sure that every value returned by std::type_info::name() is static. + typedef boost::unordered_map impl_map_type; public: - typedef LLTypeInfoLookup self; - typedef typename intf_map_type::key_type key_type; - typedef typename intf_map_type::mapped_type mapped_type; - typedef intf_value_type value_type; - - // Iterators are different because we have to transform - // impl_map_type::iterator to intf_map_type::iterator. - typedef boost::transform_iterator iterator; - typedef boost::transform_iterator const_iterator; + typedef VALUE value_type; LLTypeInfoLookup() {} - iterator begin() { return transform(mMap.begin()); } - iterator end() { return transform(mMap.end()); } - const_iterator begin() const { return const_transform(mMap.begin()); } - const_iterator end() const { return const_transform(mMap.end()); } bool empty() const { return mMap.empty(); } std::size_t size() const { return mMap.size(); } - // Shorthand -- I've always wished std::map supported this signature. - std::pair insert(const std::type_info* key, const VALUE& value) - { - return insert(value_type(key, value)); - } - - std::pair insert(const value_type& pair) + template + bool insert(const value_type& value) { // Obtain and store the std::type_info::name() string as the key. - // Save the whole incoming pair as the value! - std::pair - inserted(mMap.insert(impl_value_type(pair.first->name(), pair))); - // Have to transform the iterator before returning. - return std::pair(transform(inserted.first), inserted.second); - } - - iterator find(const std::type_info* key) - { - return transform(mMap.find(key->name())); + // Return just the bool from std::map::insert()'s return pair. + return mMap.insert(typename impl_map_type::value_type(typeid(KEY).name(), value)).second; } - const_iterator find(const std::type_info* key) const + template + boost::optional find() const { - return const_transform(mMap.find(key->name())); + // Use the std::type_info::name() string as the key. + typename impl_map_type::const_iterator found = mMap.find(typeid(KEY).name()); + if (found == mMap.end()) + return boost::optional(); + return found->second; } private: - iterator transform(impl_iterator iter) - { - return iterator(iter, boost::mem_fn(&impl_value_type::second)); - } - const_iterator const_transform(impl_const_iterator iter) - { - return const_iterator(iter, boost::mem_fn(&impl_value_type::second)); - } - impl_map_type mMap; }; -- cgit v1.2.3 From 709c1eeae90dae106800e3742f3655bd7b590b7b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Jul 2012 14:13:45 -0400 Subject: MAINT-1175: Properly pass LLRegistry's COMPARATOR to underlying map. Although LLRegistry and LLRegistrySingleton have always defined a COMPARATOR template parameter, it wasn't used for the underlying map. Therefore every type, including any pointer type, was being compared using std::less. This happens to work most of the time -- but is tripping us up now. Pass COMPARATOR to underlying std::map. Fix a couple minor bugs in LLRegistryDefaultComparator (never before used!). Specialize for const char*. Remove CompareTypeID and LLCompareTypeID because we now actively forbid using LLRegistry; remove only known reference (LLWidgetNameRegistry definition). --- indra/llcommon/llinitparam.h | 8 -------- indra/llcommon/llregistry.h | 16 +++++++++++++--- indra/llui/lluictrlfactory.h | 11 +---------- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h index c0170e533b..66c72c2d9f 100644 --- a/indra/llcommon/llinitparam.h +++ b/indra/llcommon/llinitparam.h @@ -212,14 +212,6 @@ namespace LLInitParam public: - struct CompareTypeID - { - bool operator()(const std::type_info* lhs, const std::type_info* rhs) const - { - return lhs->before(*rhs); - } - }; - typedef std::vector > name_stack_t; typedef std::pair name_stack_range_t; typedef std::vector possible_values_t; diff --git a/indra/llcommon/llregistry.h b/indra/llcommon/llregistry.h index 843c169f3d..8eeab59024 100644 --- a/indra/llcommon/llregistry.h +++ b/indra/llcommon/llregistry.h @@ -34,9 +34,19 @@ #include "llsingleton.h" template -class LLRegistryDefaultComparator +struct LLRegistryDefaultComparator { - bool operator()(const T& lhs, const T& rhs) { return lhs < rhs; } + bool operator()(const T& lhs, const T& rhs) const { return lhs < rhs; } +}; + +// comparator for const char* registry keys +template <> +struct LLRegistryDefaultComparator +{ + bool operator()(const char* lhs, const char* rhs) const + { + return strcmp(lhs, rhs) < 0; + } }; template > @@ -62,7 +72,7 @@ public: { friend class LLRegistry; public: - typedef std::map registry_map_t; + typedef std::map registry_map_t; bool add(ref_const_key_t key, ref_const_value_t value) { diff --git a/indra/llui/lluictrlfactory.h b/indra/llui/lluictrlfactory.h index 1f7a8e08ce..a5fd83e555 100644 --- a/indra/llui/lluictrlfactory.h +++ b/indra/llui/lluictrlfactory.h @@ -34,15 +34,6 @@ class LLView; -// sort functor for typeid maps -struct LLCompareTypeID -{ - bool operator()(const std::type_info* lhs, const std::type_info* rhs) const - { - return lhs->before(*rhs); - } -}; - // lookup widget constructor funcs by widget name template class LLChildRegistry : public LLRegistrySingleton @@ -71,7 +62,7 @@ protected: // lookup widget name by type (actually by std::type_info::name()) class LLWidgetNameRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; // lookup function for generating empty param block by widget type -- cgit v1.2.3 From 79a171209f41189adfeb1ba8e70c8570d380cdc5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Jul 2012 13:19:26 -0400 Subject: MAINT-1175: Linux viewer built on TC is broken, built on dev box works. Try to diagnose the cause of the misbehavior with a BOOST_STATIC_ASSERT. --- indra/llcommon/llregistry.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/indra/llcommon/llregistry.h b/indra/llcommon/llregistry.h index 8eeab59024..babc1b87aa 100644 --- a/indra/llcommon/llregistry.h +++ b/indra/llcommon/llregistry.h @@ -36,6 +36,8 @@ template struct LLRegistryDefaultComparator { + // It would be Bad if this comparison were used for const char* + BOOST_STATIC_ASSERT(! (boost::is_same::type>::type, char>::value)); bool operator()(const T& lhs, const T& rhs) const { return lhs < rhs; } }; -- cgit v1.2.3 From 55a7bdf8d3f59b0d1973d1ec22d3c8770a077723 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 16 Jul 2012 21:05:23 -0400 Subject: MAINT-1175: Pass boost::unordered_map hash/equals functors for char*. boost::unordered_map does NOT, by default, "do the right thing." Give it hash and equality functors that do. --- indra/llcommon/lltypeinfolookup.h | 44 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/indra/llcommon/lltypeinfolookup.h b/indra/llcommon/lltypeinfolookup.h index 5267e3d2fb..0b6862444e 100644 --- a/indra/llcommon/lltypeinfolookup.h +++ b/indra/llcommon/lltypeinfolookup.h @@ -13,9 +13,48 @@ #define LL_LLTYPEINFOLOOKUP_H #include +#include #include +#include // std::binary_function #include +/** + * The following helper classes are based on the Boost.Unordered documentation: + * http://www.boost.org/doc/libs/1_45_0/doc/html/unordered/hash_equality.html + */ + +/** + * Compute hash for a string passed as const char* + */ +struct const_char_star_hash: public std::unary_function +{ + std::size_t operator()(const char* str) const + { + std::size_t seed = 0; + for ( ; *str; ++str) + { + boost::hash_combine(seed, *str); + } + return seed; + } +}; + +/** + * Compute equality for strings passed as const char* + * + * I (nat) suspect that this is where the default behavior breaks for the + * const char* values returned from std::type_info::name(). If you compare the + * two const char* pointer values, as a naive, unspecialized implementation + * will surely do, they'll compare unequal. + */ +struct const_char_star_equal: public std::binary_function +{ + bool operator()(const char* lhs, const char* rhs) const + { + return strcmp(lhs, rhs) == 0; + } +}; + /** * LLTypeInfoLookup is specifically designed for use cases for which you might * consider std::map. We have several such data @@ -40,7 +79,10 @@ class LLTypeInfoLookup // std::type_info::name() string. This is one of the rare cases in which I // dare use const char* directly, rather than std::string, because I'm // sure that every value returned by std::type_info::name() is static. - typedef boost::unordered_map impl_map_type; + // HOWEVER, specify our own hash + equality functors: naively comparing + // distinct const char* values won't work. + typedef boost::unordered_map impl_map_type; public: typedef VALUE value_type; -- cgit v1.2.3 From 7f609b6a6958f519bb1becb604132b583ada3fad Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 18 Jul 2012 15:51:34 -0400 Subject: Backed out changeset a25bfa87418d (using std::type_info::name()) The changeset above touched every consumer of the two LLRegistrySingletons originally defined with std::type_info* as keys. Those two LLRegistrySingletons were changed to use const char* as keys, then all consumers were changed to pass std::type_info::name() instead of the plain std::type_info* pointer -- to deal with the observed fact that on Linux, a given type might produce different std::type_info* pointers in different load modules. Since then, Richard turned up the fascinating fact that at least some implementations of gcc's std::type_info::before() method already accommodate this peculiarity. It seems worth backing out the (dismayingly pervasive) change to see if properly using std::type_info::before() as the map comparator will work just as well, with conceptually simpler source code. This backout is transitional: we don't expect things to build/run properly until we've cherry-picked certain other pertinent changes. --- indra/llcommon/llregistry.h | 30 ++++++++++++++++++++---------- indra/llui/llfloater.cpp | 2 +- indra/llui/llradiogroup.cpp | 2 +- indra/llui/llrngwriter.cpp | 2 +- indra/llui/llscrolllistcolumn.cpp | 2 +- indra/llui/lltextbase.cpp | 2 +- indra/llui/lltooltip.cpp | 2 +- indra/llui/lluictrlfactory.cpp | 6 +++--- indra/llui/lluictrlfactory.h | 4 ++-- indra/llui/llxuiparser.cpp | 2 +- indra/llui/llxuiparser.h | 3 +-- indra/newview/llavatarlistitem.cpp | 2 +- indra/newview/llinventorylistitem.cpp | 2 +- indra/newview/llwearableitemslist.cpp | 8 ++++---- 14 files changed, 39 insertions(+), 30 deletions(-) diff --git a/indra/llcommon/llregistry.h b/indra/llcommon/llregistry.h index 843c169f3d..36d7f7a44c 100644 --- a/indra/llcommon/llregistry.h +++ b/indra/llcommon/llregistry.h @@ -30,8 +30,8 @@ #include #include -#include #include "llsingleton.h" +#include "lltypeinfolookup.h" template class LLRegistryDefaultComparator @@ -39,17 +39,27 @@ class LLRegistryDefaultComparator bool operator()(const T& lhs, const T& rhs) { return lhs < rhs; } }; +template +struct LLRegistryMapSelector +{ + typedef std::map type; +}; + +template +struct LLRegistryMapSelector +{ + typedef LLTypeInfoLookup type; +}; + +template +struct LLRegistryMapSelector +{ + typedef LLTypeInfoLookup type; +}; + template > class LLRegistry { - // Do not use LLRegistry with KEY = std::type_info* or KEY = const std::type_info*. - // This is known to fail on Linux. - // If you must use LLRegistry with dynamic type info, use KEY = const char* - // and pass std::type_info::name(); this works across load modules. - // Disallow both std::type_info* and const std::type_info*. First remove - // the pointer, then remove const, then compare is_same. - BOOST_STATIC_ASSERT(! (boost::is_same::type>::type, std::type_info>::value)); - public: typedef LLRegistry registry_t; typedef typename boost::add_reference::type>::type ref_const_key_t; @@ -62,7 +72,7 @@ public: { friend class LLRegistry; public: - typedef std::map registry_map_t; + typedef typename LLRegistryMapSelector::type registry_map_t; bool add(ref_const_key_t key, ref_const_value_t value) { diff --git a/indra/llui/llfloater.cpp b/indra/llui/llfloater.cpp index f20cc2452f..8ca1e685a9 100644 --- a/indra/llui/llfloater.cpp +++ b/indra/llui/llfloater.cpp @@ -232,7 +232,7 @@ void LLFloater::initClass() } // defaults for floater param block pulled from widgets/floater.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterFloaterParams(typeid(LLFloater::Params).name(), "floater"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterFloaterParams(&typeid(LLFloater::Params), "floater"); LLFloater::LLFloater(const LLSD& key, const LLFloater::Params& p) : LLPanel(), // intentionally do not pass params here, see initFromParams diff --git a/indra/llui/llradiogroup.cpp b/indra/llui/llradiogroup.cpp index 71bbcd8ade..95a7d09382 100644 --- a/indra/llui/llradiogroup.cpp +++ b/indra/llui/llradiogroup.cpp @@ -66,7 +66,7 @@ protected: LLSD mPayload; // stores data that this item represents in the radio group }; -static LLWidgetNameRegistry::StaticRegistrar register_radio_item(typeid(LLRadioGroup::ItemParams).name(), "radio_item"); +static LLWidgetNameRegistry::StaticRegistrar register_radio_item(&typeid(LLRadioGroup::ItemParams), "radio_item"); LLRadioGroup::Params::Params() : allow_deselect("allow_deselect"), diff --git a/indra/llui/llrngwriter.cpp b/indra/llui/llrngwriter.cpp index 90a1e77b1a..5e6840d7df 100644 --- a/indra/llui/llrngwriter.cpp +++ b/indra/llui/llrngwriter.cpp @@ -92,7 +92,7 @@ void LLRNGWriter::addDefinition(const std::string& type_name, const LLInitParam: // add includes for all possible children const std::type_info* type = *LLWidgetTypeRegistry::instance().getValue(type_name); - const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type->name()); + const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type); // add include declarations for all valid children for (widget_registry_t::Registrar::registry_map_t::const_iterator it = widget_registryp->currentRegistrar().beginItems(); diff --git a/indra/llui/llscrolllistcolumn.cpp b/indra/llui/llscrolllistcolumn.cpp index d132d0cc7e..07a6dfaa10 100644 --- a/indra/llui/llscrolllistcolumn.cpp +++ b/indra/llui/llscrolllistcolumn.cpp @@ -39,7 +39,7 @@ const S32 MIN_COLUMN_WIDTH = 20; // defaults for LLScrollColumnHeader param block pulled from widgets/scroll_column_header.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterColumnHeaderParams(typeid(LLScrollColumnHeader::Params).name(), "scroll_column_header"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterColumnHeaderParams(&typeid(LLScrollColumnHeader::Params), "scroll_column_header"); //--------------------------------------------------------------------------- // LLScrollColumnHeader diff --git a/indra/llui/lltextbase.cpp b/indra/llui/lltextbase.cpp index 8314386632..7aeeae298f 100644 --- a/indra/llui/lltextbase.cpp +++ b/indra/llui/lltextbase.cpp @@ -131,7 +131,7 @@ struct LLTextBase::line_end_compare // // register LLTextBase::Params under name "textbase" -static LLWidgetNameRegistry::StaticRegistrar sRegisterTextBaseParams(typeid(LLTextBase::Params).name(), "textbase"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterTextBaseParams(&typeid(LLTextBase::Params), "textbase"); LLTextBase::LineSpacingParams::LineSpacingParams() : multiple("multiple", 1.f), diff --git a/indra/llui/lltooltip.cpp b/indra/llui/lltooltip.cpp index 0723dbec70..f737d48abf 100644 --- a/indra/llui/lltooltip.cpp +++ b/indra/llui/lltooltip.cpp @@ -129,7 +129,7 @@ void LLToolTipView::drawStickyRect() } // defaults for floater param block pulled from widgets/floater.xml -static LLWidgetNameRegistry::StaticRegistrar sRegisterInspectorParams(typeid(LLInspector::Params).name(), "inspector"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterInspectorParams(&typeid(LLInspector::Params), "inspector"); // // LLToolTip diff --git a/indra/llui/lluictrlfactory.cpp b/indra/llui/lluictrlfactory.cpp index fe3b470722..25e7a31e90 100644 --- a/indra/llui/lluictrlfactory.cpp +++ b/indra/llui/lluictrlfactory.cpp @@ -281,7 +281,7 @@ const LLInitParam::BaseBlock& get_empty_param_block() void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const std::type_info* param_block_type, const std::string& tag) { // associate parameter block type with template .xml file - std::string* existing_tag = LLWidgetNameRegistry::instance().getValue(param_block_type->name()); + std::string* existing_tag = LLWidgetNameRegistry::instance().getValue(param_block_type); if (existing_tag != NULL) { if(*existing_tag != tag) @@ -297,7 +297,7 @@ void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const st return; } } - LLWidgetNameRegistry::instance().defaultRegistrar().add(param_block_type->name(), tag); + LLWidgetNameRegistry::instance().defaultRegistrar().add(param_block_type, tag); //FIXME: comment this in when working on schema generation //LLWidgetTypeRegistry::instance().defaultRegistrar().add(tag, widget_type); //LLDefaultParamBlockRegistry::instance().defaultRegistrar().add(widget_type, &get_empty_param_block); @@ -306,6 +306,6 @@ void LLUICtrlFactory::registerWidget(const std::type_info* widget_type, const st //static const std::string* LLUICtrlFactory::getWidgetTag(const std::type_info* widget_type) { - return LLWidgetNameRegistry::instance().getValue(widget_type->name()); + return LLWidgetNameRegistry::instance().getValue(widget_type); } diff --git a/indra/llui/lluictrlfactory.h b/indra/llui/lluictrlfactory.h index 1f7a8e08ce..d612ad5005 100644 --- a/indra/llui/lluictrlfactory.h +++ b/indra/llui/lluictrlfactory.h @@ -69,9 +69,9 @@ protected: friend class LLSingleton; }; -// lookup widget name by type (actually by std::type_info::name()) +// lookup widget name by type class LLWidgetNameRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; // lookup function for generating empty param block by widget type diff --git a/indra/llui/llxuiparser.cpp b/indra/llui/llxuiparser.cpp index c3a246b9a5..afc76024d1 100644 --- a/indra/llui/llxuiparser.cpp +++ b/indra/llui/llxuiparser.cpp @@ -606,7 +606,7 @@ void LLXUIXSDWriter::writeXSD(const std::string& type_name, const std::string& p // add includes for all possible children const std::type_info* type = *LLWidgetTypeRegistry::instance().getValue(type_name); - const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type->name()); + const widget_registry_t* widget_registryp = LLChildRegistryRegistry::instance().getValue(type); // add choices for valid children if (widget_registryp) diff --git a/indra/llui/llxuiparser.h b/indra/llui/llxuiparser.h index 3c092afdd6..d7cd256967 100644 --- a/indra/llui/llxuiparser.h +++ b/indra/llui/llxuiparser.h @@ -55,9 +55,8 @@ typedef boost::function widget_registry_t; -// const char* key used for std::type_info::name() string class LLChildRegistryRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; diff --git a/indra/newview/llavatarlistitem.cpp b/indra/newview/llavatarlistitem.cpp index c07fec8e5e..30eecfe323 100644 --- a/indra/newview/llavatarlistitem.cpp +++ b/indra/newview/llavatarlistitem.cpp @@ -44,7 +44,7 @@ S32 LLAvatarListItem::sLeftPadding = 0; S32 LLAvatarListItem::sNameRightPadding = 0; S32 LLAvatarListItem::sChildrenWidths[LLAvatarListItem::ALIC_COUNT]; -static LLWidgetNameRegistry::StaticRegistrar sRegisterAvatarListItemParams(typeid(LLAvatarListItem::Params).name(), "avatar_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterAvatarListItemParams(&typeid(LLAvatarListItem::Params), "avatar_list_item"); LLAvatarListItem::Params::Params() : default_style("default_style"), diff --git a/indra/newview/llinventorylistitem.cpp b/indra/newview/llinventorylistitem.cpp index 5489671430..3e0849a795 100644 --- a/indra/newview/llinventorylistitem.cpp +++ b/indra/newview/llinventorylistitem.cpp @@ -40,7 +40,7 @@ #include "llinventorymodel.h" #include "llviewerinventory.h" -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelInventoryListItemBaseParams(typeid(LLPanelInventoryListItemBase::Params).name(), "inventory_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelInventoryListItemBaseParams(&typeid(LLPanelInventoryListItemBase::Params), "inventory_list_item"); static const S32 WIDGET_SPACING = 3; diff --git a/indra/newview/llwearableitemslist.cpp b/indra/newview/llwearableitemslist.cpp index abb53301a6..92697fb2eb 100644 --- a/indra/newview/llwearableitemslist.cpp +++ b/indra/newview/llwearableitemslist.cpp @@ -135,7 +135,7 @@ void LLPanelWearableOutfitItem::updateItem(const std::string& name, ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelClothingListItem(typeid(LLPanelClothingListItem::Params).name(), "clothing_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelClothingListItem(&typeid(LLPanelClothingListItem::Params), "clothing_list_item"); LLPanelClothingListItem::Params::Params() @@ -222,7 +222,7 @@ BOOL LLPanelClothingListItem::postBuild() ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelBodyPartsListItem(typeid(LLPanelBodyPartsListItem::Params).name(), "bodyparts_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelBodyPartsListItem(&typeid(LLPanelBodyPartsListItem::Params), "bodyparts_list_item"); LLPanelBodyPartsListItem::Params::Params() @@ -293,7 +293,7 @@ BOOL LLPanelBodyPartsListItem::postBuild() return TRUE; } -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDeletableWearableListItem(typeid(LLPanelDeletableWearableListItem::Params).name(), "deletable_wearable_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDeletableWearableListItem(&typeid(LLPanelDeletableWearableListItem::Params), "deletable_wearable_list_item"); LLPanelDeletableWearableListItem::Params::Params() : delete_btn("delete_btn") @@ -373,7 +373,7 @@ void LLPanelAttachmentListItem::updateItem(const std::string& name, ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// ////////////////////////////////////////////////////////////////////////// -static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDummyClothingListItem(typeid(LLPanelDummyClothingListItem::Params).name(), "dummy_clothing_list_item"); +static LLWidgetNameRegistry::StaticRegistrar sRegisterPanelDummyClothingListItem(&typeid(LLPanelDummyClothingListItem::Params), "dummy_clothing_list_item"); LLPanelDummyClothingListItem::Params::Params() : add_panel("add_panel"), -- cgit v1.2.3 From 2e83dfa217feb90e7b94e499346ad9b98fa711b2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 18 Jul 2012 20:33:54 -0400 Subject: MAINT-1175: Ditch LLTypeInfoLookup, make map work. Instead of forbidding std::map outright (which includes LLRegistry and LLRegistrySingleton), try to make it work by specializing std::less to use std::type_info::before(). Make LLRegistryDefaultComparator use std::less so it can capitalize on that specialization. --- indra/llcommon/llinitparam.h | 32 ++++++++++++++++---------------- indra/llcommon/llregistry.h | 34 ++++------------------------------ indra/llcommon/llstl.h | 30 ++++++++++++++++++++++++++++++ indra/llui/lluictrlfactory.h | 5 +++-- 4 files changed, 53 insertions(+), 48 deletions(-) diff --git a/indra/llcommon/llinitparam.h b/indra/llcommon/llinitparam.h index 66c72c2d9f..9a6d1eff5c 100644 --- a/indra/llcommon/llinitparam.h +++ b/indra/llcommon/llinitparam.h @@ -35,7 +35,7 @@ #include #include "llerror.h" -#include "lltypeinfolookup.h" +#include "llstl.h" namespace LLInitParam { @@ -220,9 +220,9 @@ namespace LLInitParam typedef bool (*parser_write_func_t)(Parser& parser, const void*, name_stack_t&); typedef boost::function parser_inspect_func_t; - typedef LLTypeInfoLookup parser_read_func_map_t; - typedef LLTypeInfoLookup parser_write_func_map_t; - typedef LLTypeInfoLookup parser_inspect_func_map_t; + typedef std::map parser_read_func_map_t; + typedef std::map parser_write_func_map_t; + typedef std::map parser_inspect_func_map_t; Parser(parser_read_func_map_t& read_map, parser_write_func_map_t& write_map, parser_inspect_func_map_t& inspect_map) : mParseSilently(false), @@ -234,20 +234,20 @@ namespace LLInitParam template bool readValue(T& param) { - boost::optional found_it = mParserReadFuncs->find(); - if (found_it) + parser_read_func_map_t::iterator found_it = mParserReadFuncs->find(&typeid(T)); + if (found_it != mParserReadFuncs->end()) { - return (*found_it)(*this, (void*)¶m); + return found_it->second(*this, (void*)¶m); } return false; } template bool writeValue(const T& param, name_stack_t& name_stack) { - boost::optional found_it = mParserWriteFuncs->find(); - if (found_it) + parser_write_func_map_t::iterator found_it = mParserWriteFuncs->find(&typeid(T)); + if (found_it != mParserWriteFuncs->end()) { - return (*found_it)(*this, (const void*)¶m, name_stack); + return found_it->second(*this, (const void*)¶m, name_stack); } return false; } @@ -255,10 +255,10 @@ namespace LLInitParam // dispatch inspection to registered inspection functions, for each parameter in a param block template bool inspectValue(name_stack_t& name_stack, S32 min_count, S32 max_count, const possible_values_t* possible_values) { - boost::optional found_it = mParserInspectFuncs->find(); - if (found_it) + parser_inspect_func_map_t::iterator found_it = mParserInspectFuncs->find(&typeid(T)); + if (found_it != mParserInspectFuncs->end()) { - (*found_it)(name_stack, min_count, max_count, possible_values); + found_it->second(name_stack, min_count, max_count, possible_values); return true; } return false; @@ -273,14 +273,14 @@ namespace LLInitParam template void registerParserFuncs(parser_read_func_t read_func, parser_write_func_t write_func = NULL) { - mParserReadFuncs->insert(read_func); - mParserWriteFuncs->insert(write_func); + mParserReadFuncs->insert(std::make_pair(&typeid(T), read_func)); + mParserWriteFuncs->insert(std::make_pair(&typeid(T), write_func)); } template void registerInspectFunc(parser_inspect_func_t inspect_func) { - mParserInspectFuncs->insert(inspect_func); + mParserInspectFuncs->insert(std::make_pair(&typeid(T), inspect_func)); } bool mParseSilently; diff --git a/indra/llcommon/llregistry.h b/indra/llcommon/llregistry.h index 2df9bc6541..853c427a13 100644 --- a/indra/llcommon/llregistry.h +++ b/indra/llcommon/llregistry.h @@ -31,44 +31,18 @@ #include #include "llsingleton.h" -#include "lltypeinfolookup.h" +#include "llstl.h" template struct LLRegistryDefaultComparator { - // It would be Bad if this comparison were used for const char* - BOOST_STATIC_ASSERT(! (boost::is_same::type>::type, char>::value)); - bool operator()(const T& lhs, const T& rhs) const { return lhs < rhs; } -}; - -// comparator for const char* registry keys -template <> -struct LLRegistryDefaultComparator -{ - bool operator()(const char* lhs, const char* rhs) const + bool operator()(const T& lhs, const T& rhs) const { - return strcmp(lhs, rhs) < 0; + using std::less; + return less()(lhs, rhs); } }; -template -struct LLRegistryMapSelector -{ - typedef std::map type; -}; - -template -struct LLRegistryMapSelector -{ - typedef LLTypeInfoLookup type; -}; - -template -struct LLRegistryMapSelector -{ - typedef LLTypeInfoLookup type; -}; - template > class LLRegistry { diff --git a/indra/llcommon/llstl.h b/indra/llcommon/llstl.h index 8ad12c9a03..6109b21546 100644 --- a/indra/llcommon/llstl.h +++ b/indra/llcommon/llstl.h @@ -33,6 +33,7 @@ #include #include #include +#include // Use to compare the first element only of a pair // e.g. typedef std::set, compare_pair > some_pair_set_t; @@ -470,4 +471,33 @@ llbind2nd(const _Operation& __oper, const _Tp& __x) return llbinder2nd<_Operation>(__oper, _Arg2_type(__x)); } +/** + * Specialize std::less to use std::type_info::before(). + * See MAINT-1175. It is NEVER a good idea to directly compare std::type_info* + * because, on Linux, you might get different std::type_info* pointers for the + * same type (from different load modules)! + */ +namespace std +{ + template <> + struct less: + public std::binary_function + { + bool operator()(const std::type_info* lhs, const std::type_info* rhs) const + { + return lhs->before(*rhs); + } + }; + + template <> + struct less: + public std::binary_function + { + bool operator()(std::type_info* lhs, std::type_info* rhs) const + { + return lhs->before(*rhs); + } + }; +} // std + #endif // LL_LLSTL_H diff --git a/indra/llui/lluictrlfactory.h b/indra/llui/lluictrlfactory.h index ab16805cb1..4e54354731 100644 --- a/indra/llui/lluictrlfactory.h +++ b/indra/llui/lluictrlfactory.h @@ -31,6 +31,7 @@ #include "llinitparam.h" #include "llregistry.h" #include "llxuiparser.h" +#include "llstl.h" class LLView; @@ -62,14 +63,14 @@ protected: // lookup widget name by type class LLWidgetNameRegistry -: public LLRegistrySingleton +: public LLRegistrySingleton {}; // lookup function for generating empty param block by widget type // this is used for schema generation //typedef const LLInitParam::BaseBlock& (*empty_param_block_func_t)(); //class LLDefaultParamBlockRegistry -//: public LLRegistrySingleton +//: public LLRegistrySingleton //{}; extern LLFastTimer::DeclareTimer FTM_WIDGET_SETUP; -- cgit v1.2.3 From 27cbfef0269146eb296bbd2b810a6d331e5cb7d2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 19 Jul 2012 08:35:11 -0400 Subject: MAINT-1175: Use workaround for comparing std::type_info* on gcc < 4.4. We now specialize std::less to use std::type_info::before(), and on Windows and Mac that Just Works. It even works on Linux when using gcc 4.4+: more recent implementations of gcc's std::type_info::before() apparently do name()-string comparisons internally. It doesn't work so well on Linux with gcc 4.1, though, and that's the compiler we still use on our Linux build-farm machines. But rather than give up, perform explicit name()-string comparison in that case. --- indra/llcommon/llstl.h | 59 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/indra/llcommon/llstl.h b/indra/llcommon/llstl.h index 6109b21546..d3941e1bc9 100644 --- a/indra/llcommon/llstl.h +++ b/indra/llcommon/llstl.h @@ -471,6 +471,27 @@ llbind2nd(const _Operation& __oper, const _Tp& __x) return llbinder2nd<_Operation>(__oper, _Arg2_type(__x)); } +/** + * Compare std::type_info* pointers a la std::less. We break this out as a + * separate function for use in two different std::less specializations. + */ +inline +bool before(const std::type_info* lhs, const std::type_info* rhs) +{ +#if LL_LINUX && defined(__GNUC__) && ((__GNUC__ < 4) || (__GNUC__ == 4 && __GNUC_MINOR__ < 4)) + // If we're building on Linux with gcc, and it's either gcc 3.x or + // 4.{0,1,2,3}, then we have to use a workaround. Note that we use gcc on + // Mac too, and some people build with gcc on Windows (cygwin or mingw). + // On Linux, different load modules may produce different type_info* + // pointers for the same type. Have to compare name strings to get good + // results. + return strcmp(lhs->name(), rhs->name()) < 0; +#else // not Linux, or gcc 4.4+ + // Just use before(), as we normally would + return lhs->before(*rhs); +#endif +} + /** * Specialize std::less to use std::type_info::before(). * See MAINT-1175. It is NEVER a good idea to directly compare std::type_info* @@ -479,25 +500,25 @@ llbind2nd(const _Operation& __oper, const _Tp& __x) */ namespace std { - template <> - struct less: - public std::binary_function - { - bool operator()(const std::type_info* lhs, const std::type_info* rhs) const - { - return lhs->before(*rhs); - } - }; - - template <> - struct less: - public std::binary_function - { - bool operator()(std::type_info* lhs, std::type_info* rhs) const - { - return lhs->before(*rhs); - } - }; + template <> + struct less: + public std::binary_function + { + bool operator()(const std::type_info* lhs, const std::type_info* rhs) const + { + return before(lhs, rhs); + } + }; + + template <> + struct less: + public std::binary_function + { + bool operator()(std::type_info* lhs, std::type_info* rhs) const + { + return before(lhs, rhs); + } + }; } // std #endif // LL_LLSTL_H -- cgit v1.2.3 From fd7bfb03c946b0190bf0965e19b1b679eac3c383 Mon Sep 17 00:00:00 2001 From: Richard Linden Date: Fri, 27 Jul 2012 15:18:29 -0700 Subject: MAINT-1175 FIX Severe performance issues on 3.3.2 (258114) Release and 3.3.3 (259197) Beta in "Add scroll list item" removed linear scan for namelist items and used weak reference (LLHandle) instead --- indra/newview/llnamelistctrl.cpp | 20 +++++++++----------- indra/newview/llnamelistctrl.h | 41 ++++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/indra/newview/llnamelistctrl.cpp b/indra/newview/llnamelistctrl.cpp index 4e28d1f526..5029d26fda 100644 --- a/indra/newview/llnamelistctrl.cpp +++ b/indra/newview/llnamelistctrl.cpp @@ -336,7 +336,7 @@ LLScrollListItem* LLNameListCtrl::addNameItemRow( // ...schedule a callback LLAvatarNameCache::get(id, boost::bind(&LLNameListCtrl::onAvatarNameCache, - this, _1, _2)); + this, _1, _2, item->getHandle())); } break; } @@ -392,7 +392,8 @@ void LLNameListCtrl::removeNameItem(const LLUUID& agent_id) } void LLNameListCtrl::onAvatarNameCache(const LLUUID& agent_id, - const LLAvatarName& av_name) + const LLAvatarName& av_name, + LLHandle item) { std::string name; if (mShortNames) @@ -400,17 +401,14 @@ void LLNameListCtrl::onAvatarNameCache(const LLUUID& agent_id, else name = av_name.getCompleteName(); - item_list::iterator iter; - for (iter = getItemList().begin(); iter != getItemList().end(); iter++) + LLNameListItem* list_item = item.get(); + if (list_item && list_item->getUUID() == agent_id) { - LLScrollListItem* item = *iter; - if (item->getUUID() == agent_id) + LLScrollListCell* cell = list_item->getColumn(mNameColumnIndex); + if (cell) { - LLScrollListCell* cell = item->getColumn(mNameColumnIndex); - if (cell) - { - cell->setValue(name); - } + cell->setValue(name); + setNeedsSort(); } } diff --git a/indra/newview/llnamelistctrl.h b/indra/newview/llnamelistctrl.h index ca9956dc53..ffacd69db4 100644 --- a/indra/newview/llnamelistctrl.h +++ b/indra/newview/llnamelistctrl.h @@ -33,6 +33,26 @@ class LLAvatarName; +/** + * LLNameListCtrl item + * + * We don't use LLScrollListItem to be able to override getUUID(), which is needed + * because the name list item value is not simply an UUID but a map (uuid, is_group). + */ +class LLNameListItem : public LLScrollListItem, public LLHandleProvider +{ +public: + LLUUID getUUID() const { return getValue()["uuid"].asUUID(); } +protected: + friend class LLNameListCtrl; + + LLNameListItem( const LLScrollListItem::Params& p ) + : LLScrollListItem(p) + { + } +}; + + class LLNameListCtrl : public LLScrollListCtrl, public LLInstanceTracker { @@ -115,7 +135,7 @@ public: /*virtual*/ void mouseOverHighlightNthItem( S32 index ); private: void showInspector(const LLUUID& avatar_id, bool is_group); - void onAvatarNameCache(const LLUUID& agent_id, const LLAvatarName& av_name); + void onAvatarNameCache(const LLUUID& agent_id, const LLAvatarName& av_name, LLHandle item); private: S32 mNameColumnIndex; @@ -124,24 +144,5 @@ private: bool mShortNames; // display name only, no SLID }; -/** - * LLNameListCtrl item - * - * We don't use LLScrollListItem to be able to override getUUID(), which is needed - * because the name list item value is not simply an UUID but a map (uuid, is_group). - */ -class LLNameListItem : public LLScrollListItem -{ -public: - LLUUID getUUID() const { return getValue()["uuid"].asUUID(); } - -protected: - friend class LLNameListCtrl; - - LLNameListItem( const LLScrollListItem::Params& p ) - : LLScrollListItem(p) - { - } -}; #endif -- cgit v1.2.3