[clang-tools-extra] r318840 - [FindAllSymbols] Cache regexes, creating them is expensive

Krzysztof Parzyszek via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 23 15:01:31 PST 2017


+Dimitry.


On 11/23/2017 12:50 PM, Benjamin Kramer wrote:
> I'm afraid I can't really help you here. You can try twiddling the
> code a bit by creating the pair explicitly and moving it into the
> vector, just to avoid the brokenness in the standard library. Not sure
> if that will help though.
> 
> On Thu, Nov 23, 2017 at 7:05 PM, Krzysztof Parzyszek
> <kparzysz at codeaurora.org> wrote:
>> There has been some problem with std::pair on FreeBSD (due to some ABI
>> compatibility issue), but I don't know much about what it was.
>>
>> Commenting the reserve doesn't help, unfortunately. The same problem is now
>> flagged in emplace_back.
>>
>> -Krzysztof
>>
>>
>>
>> On 11/23/2017 11:47 AM, Benjamin Kramer wrote:
>>>
>>> That looks like a bug in the standard library. Does removing the call
>>> to reserve fix it? It's not really necessary, that code isn't
>>> performance sensitive at all.
>>>
>>> On Thu, Nov 23, 2017 at 6:36 PM, Krzysztof Parzyszek
>>> <kparzysz at codeaurora.org> wrote:
>>>>
>>>> Hi,
>>>> This broke build on FreeBSD 11:
>>>>
>>>> [100%] Building CXX object
>>>>
>>>> tools/clang/tools/extra/include-fixer/find-all-symbols/CMakeFiles/findAllSymbols.dir/HeaderMapCollector.cpp.o
>>>> cd /w/bld/org/tools/clang/tools/extra/include-fixer/find-all-symbols &&
>>>> /w/c/clang+llvm-5.0.0-x86_64-unknown-freebsd11/bin/clang++
>>>> -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
>>>> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>>> -I/w/bld/org/tools/clang/tools/extra/include-fixer/find-all-symbols
>>>> -I/w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols
>>>> -I/w/src/llvm.org/tools/clang/include -I/w/bld/org/tools/clang/include
>>>> -I/w/bld/org/include -I/w/src/llvm.org/include -isystem
>>>> /usr/local/include
>>>> -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time
>>>> -Werror=unguarded-availability-new -std=c++11 -Wall -W
>>>> -Wno-unused-parameter
>>>> -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic
>>>> -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor
>>>> -Wdelete-non-virtual-dtor -Wno-comment -Wstring-conversion
>>>> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual
>>>> -Wno-nested-anon-types -O3    -UNDEBUG -fno-exceptions -fno-rtti -o
>>>> CMakeFiles/findAllSymbols.dir/HeaderMapCollector.cpp.o -c
>>>>
>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>> In file included from
>>>>
>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp:10:
>>>> In file included from
>>>>
>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.h:13:
>>>> In file included from /w/src/llvm.org/include/llvm/ADT/StringMap.h:17:
>>>> In file included from /w/src/llvm.org/include/llvm/ADT/StringRef.h:13:
>>>> In file included from /w/src/llvm.org/include/llvm/ADT/STLExtras.h:20:
>>>> In file included from /w/src/llvm.org/include/llvm/ADT/Optional.h:22:
>>>> In file included from
>>>> /w/src/llvm.org/include/llvm/Support/type_traits.h:19:
>>>> /usr/include/c++/v1/utility:315:11: error: call to deleted constructor of
>>>> 'llvm::Regex'
>>>>           : first(__p.first),
>>>>             ^     ~~~~~~~~~
>>>> /usr/include/c++/v1/memory:1747:31: note: in instantiation of member
>>>> function 'std::__1::pair<llvm::Regex, const char *>::pair' requested here
>>>>               ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
>>>>                                 ^
>>>> /usr/include/c++/v1/memory:1658:18: note: in instantiation of function
>>>> template specialization 'std::__1::allocator<std::__1::pair<llvm::Regex,
>>>> const char *> >::construct<std::__1::pair<llvm::Regex, const char *>,
>>>> const
>>>> std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>               {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}
>>>>                    ^
>>>> /usr/include/c++/v1/memory:1504:14: note: in instantiation of function
>>>> template specialization
>>>>
>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>> const char *> > >::__construct<std::__1::pair<llvm::Regex, const char *>,
>>>> const std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>               {__construct(__has_construct<allocator_type, _Tp*,
>>>> _Args...>(),
>>>>                ^
>>>> /usr/include/c++/v1/memory:1620:17: note: in instantiation of function
>>>> template specialization
>>>>
>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>> const char *> > >::construct<std::__1::pair<llvm::Regex, const char *>,
>>>> const std::__1::pair<llvm::Regex, const char *> &>' requested here
>>>>                   construct(__a, _VSTD::__to_raw_pointer(__end2-1),
>>>> _VSTD::move_if_noexcept(*--__end1));
>>>>                   ^
>>>> /usr/include/c++/v1/vector:892:21: note: in instantiation of function
>>>> template specialization
>>>>
>>>> 'std::__1::allocator_traits<std::__1::allocator<std::__1::pair<llvm::Regex,
>>>> const char *> > >::__construct_backward<std::__1::pair<llvm::Regex, const
>>>> char *> *>' requested here
>>>>       __alloc_traits::__construct_backward(this->__alloc(),
>>>> this->__begin_,
>>>> this->__end_, __v.__begin_);
>>>>                       ^
>>>> /usr/include/c++/v1/vector:1537:9: note: in instantiation of member
>>>> function
>>>> 'std::__1::vector<std::__1::pair<llvm::Regex, const char *>,
>>>> std::__1::allocator<std::__1::pair<llvm::Regex, const char *> >
>>>>>
>>>>> ::__swap_out_circular_buffer' requested here
>>>>
>>>>           __swap_out_circular_buffer(__v);
>>>>           ^
>>>>
>>>> /w/src/llvm.org/tools/clang/tools/extra/include-fixer/find-all-symbols/HeaderMapCollector.cpp:19:33:
>>>> note: in instantiation of member function
>>>> 'std::__1::vector<std::__1::pair<llvm::Regex, const char *>,
>>>> std::__1::allocator<std::__1::pair<llvm::Regex, const char *> >
>>>>> ::reserve'
>>>> requested here
>>>>
>>>> this->RegexHeaderMappingTable.reserve(RegexHeaderMappingTable->size());
>>>>                                   ^
>>>> /w/src/llvm.org/include/llvm/Support/Regex.h:49:5: note: 'Regex' has been
>>>> explicitly marked deleted here
>>>>       Regex(const Regex &) = delete;
>>>>       ^
>>>> 1 error generated.
>>>> *** Error code 1
>>>>
>>>>
>>>> -Krzysztof
>>>>
>>>>
>>>>
>>>> On 11/22/2017 9:38 AM, Benjamin Kramer via cfe-commits wrote:
>>>>>
>>>>>
>>>>> Author: d0k
>>>>> Date: Wed Nov 22 07:38:23 2017
>>>>> New Revision: 318840
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=318840&view=rev
>>>>> Log:
>>>>> [FindAllSymbols] Cache regexes, creating them is expensive
>>>>>
>>>>> This is a bit annoying because LLVM regexes are always mutable to store
>>>>> errors. Assert that there are never errors and fix broken hardcoded
>>>>> regexes.
>>>>>
>>>>> Modified:
>>>>>
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>>
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>>
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>>
>>>>> Modified:
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>> URL:
>>>>>
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp?rev=318840&r1=318839&r2=318840&view=diff
>>>>>
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>> (original)
>>>>> +++
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.cpp
>>>>> Wed Nov 22 07:38:23 2017
>>>>> @@ -13,6 +13,16 @@
>>>>>     namespace clang {
>>>>>     namespace find_all_symbols {
>>>>>     +HeaderMapCollector::HeaderMapCollector(
>>>>> +    const RegexHeaderMap *RegexHeaderMappingTable) {
>>>>> +  assert(RegexHeaderMappingTable);
>>>>> +
>>>>> this->RegexHeaderMappingTable.reserve(RegexHeaderMappingTable->size());
>>>>> +  for (const auto &Entry : *RegexHeaderMappingTable) {
>>>>> +
>>>>> this->RegexHeaderMappingTable.emplace_back(llvm::Regex(Entry.first),
>>>>> +                                               Entry.second);
>>>>> +  }
>>>>> +}
>>>>> +
>>>>>     llvm::StringRef
>>>>>     HeaderMapCollector::getMappedHeader(llvm::StringRef Header) const {
>>>>>       auto Iter = HeaderMappingTable.find(Header);
>>>>> @@ -20,11 +30,13 @@ HeaderMapCollector::getMappedHeader(llvm
>>>>>         return Iter->second;
>>>>>       // If there is no complete header name mapping for this header,
>>>>> check
>>>>> the
>>>>>       // regex header mapping.
>>>>> -  if (RegexHeaderMappingTable) {
>>>>> -    for (const auto &Entry : *RegexHeaderMappingTable) {
>>>>> -      if (llvm::Regex(Entry.first).match(Header))
>>>>> -        return Entry.second;
>>>>> -    }
>>>>> +  for (auto &Entry : RegexHeaderMappingTable) {
>>>>> +#ifndef NDEBUG
>>>>> +    std::string Dummy;
>>>>> +    assert(Entry.first.isValid(Dummy) && "Regex should never be
>>>>> invalid!");
>>>>> +#endif
>>>>> +    if (Entry.first.match(Header))
>>>>> +      return Entry.second;
>>>>>       }
>>>>>       return Header;
>>>>>     }
>>>>>
>>>>> Modified:
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>> URL:
>>>>>
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h?rev=318840&r1=318839&r2=318840&view=diff
>>>>>
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>> (original)
>>>>> +++
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/HeaderMapCollector.h
>>>>> Wed Nov 22 07:38:23 2017
>>>>> @@ -25,10 +25,8 @@ public:
>>>>>       typedef llvm::StringMap<std::string> HeaderMap;
>>>>>       typedef std::vector<std::pair<const char *, const char *>>
>>>>> RegexHeaderMap;
>>>>>     -  HeaderMapCollector() : RegexHeaderMappingTable(nullptr) {}
>>>>> -
>>>>> -  explicit HeaderMapCollector(const RegexHeaderMap
>>>>> *RegexHeaderMappingTable)
>>>>> -      : RegexHeaderMappingTable(RegexHeaderMappingTable) {}
>>>>> +  HeaderMapCollector() = default;
>>>>> +  explicit HeaderMapCollector(const RegexHeaderMap
>>>>> *RegexHeaderMappingTable);
>>>>>         void addHeaderMapping(llvm::StringRef OrignalHeaderPath,
>>>>>                             llvm::StringRef MappingHeaderPath) {
>>>>> @@ -47,8 +45,10 @@ private:
>>>>>       HeaderMap HeaderMappingTable;
>>>>>         // A map from header patterns to header names.
>>>>> -  // This is a reference to a hard-coded map.
>>>>> -  const RegexHeaderMap *const RegexHeaderMappingTable;
>>>>> +  // The header names are not owned. This is only threadsafe because
>>>>> the
>>>>> regexes
>>>>> +  // never fail.
>>>>> +  mutable std::vector<std::pair<llvm::Regex, const char *>>
>>>>> +      RegexHeaderMappingTable;
>>>>>     };
>>>>>       } // namespace find_all_symbols
>>>>>
>>>>> Modified:
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>> URL:
>>>>>
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp?rev=318840&r1=318839&r2=318840&view=diff
>>>>>
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>> (original)
>>>>> +++
>>>>>
>>>>> clang-tools-extra/trunk/include-fixer/find-all-symbols/STLPostfixHeaderMap.cpp
>>>>> Wed Nov 22 07:38:23 2017
>>>>> @@ -314,10 +314,10 @@ const HeaderMapCollector::RegexHeaderMap
>>>>>           {"include/xlocale.h$", "<cstring>"},
>>>>>           {"bits/atomic_word.h$", "<memory>"},
>>>>>           {"bits/basic_file.h$", "<fstream>"},
>>>>> -      {"bits/c++allocator.h$", "<string>"},
>>>>> -      {"bits/c++config.h$", "<iosfwd>"},
>>>>> -      {"bits/c++io.h$", "<ios>"},
>>>>> -      {"bits/c++locale.h$", "<locale>"},
>>>>> +      {"bits/c\\+\\+allocator.h$", "<string>"},
>>>>> +      {"bits/c\\+\\+config.h$", "<iosfwd>"},
>>>>> +      {"bits/c\\+\\+io.h$", "<ios>"},
>>>>> +      {"bits/c\\+\\+locale.h$", "<locale>"},
>>>>>           {"bits/cpu_defines.h$", "<iosfwd>"},
>>>>>           {"bits/ctype_base.h$", "<locale>"},
>>>>>           {"bits/cxxabi_tweaks.h$", "<cxxabi.h>"},
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>> ---
>>>>> This email has been checked for viruses by AVG.
>>>>> http://www.avg.com
>>>>>
>>>>
>>



More information about the cfe-commits mailing list