Page MenuHomePhabricator

Load lazily the template specialization in multi-module setups.
ClosedPublic

Authored by v.g.vassilev on Feb 14 2017, 11:01 AM.

Details

Reviewers
rsmith
Summary

Currently, we load all template specialization if we have more than one module attached and we touch anything around the template definition.

This patch registers the template specializations as a lazily-loadable entities. This reduces the amount of deserializations by 1%.

Diff Detail

Event Timeline

v.g.vassilev created this revision.Feb 14 2017, 11:01 AM
aprantl added inline comments.
lib/Serialization/ASTReaderDecl.cpp
3949

llvm_unreachable()

v.g.vassilev marked an inline comment as done.

Reduce code duplication. Use llvm_unreachable.

rsmith added inline comments.May 9 2017, 12:03 PM
lib/Serialization/ASTReaderDecl.cpp
213–215

Can this ever fail at runtime? I'd expect the below code to not compile if D isn't one of these types.

3926–3928

This will end up being quadratic time and using quadratic storage if a module adds N specializations to a template; instead, please move the SmallVector out of the loop and call AddLazySpecializations once after reading all the update records for the declaration. (It's probably best to move it all the way out to loadDeclUpdateRecords, in case we have a large number of modules each adding some specializations -- eg, if module A imports std::vector and declares A, module B declares B and uses vector<A>, module C declares C and uses vector<B>, ..., we again should avoid quadratic behavior.)

v.g.vassilev marked 2 inline comments as done.

Reduce the amount of calls to AddLazySpecializations. Remove assert.

In order to create a reasonable test I need to use -error-on-deserialized-decl and I hit a bug: https://bugs.llvm.org/show_bug.cgi?id=32988

Richard, could you help me out with the test? Maybe we could trigger this avoiding the abovementioned bug...

rsmith added inline comments.May 23 2017, 12:07 PM
include/clang/Serialization/ASTReader.h
558

I'm not sure we can safely use global state for this: loading update records can trigger the import of a declaration and *its* update records, which would mean we'd attach the pending lazy specializations to the wrong declaration. Can you keep this list locally in ASTReader::loadDeclUpdateRecords instead, and pass it into UpdateDecl?

1297–1299

Make this a static member of ASTDeclReader to avoid this hack.

rsmith edited edge metadata.May 23 2017, 1:18 PM

In order to create a reasonable test I need to use -error-on-deserialized-decl and I hit a bug: https://bugs.llvm.org/show_bug.cgi?id=32988

Richard, could you help me out with the test? Maybe we could trigger this avoiding the abovementioned bug...

You could try to test this using -dump-deserialized-decls instead, but it'd be good to figure out what's going wrong in the above bug :) Nothing in the backtrace looks at all related to -dump-deserialized-decls, but maybe we're messing up our internal state by calling into the DeserializationListeners at an unsafe point during deserialization?

v.g.vassilev marked 2 inline comments as done.

Address comments.

rsmith accepted this revision.Jun 9 2017, 12:31 PM

Looks great, thanks!

This revision is now accepted and ready to land.Jun 9 2017, 12:31 PM

Reverted in r305460 because broke libcxx modules builds.

For the record: relanded in r306903.