Introduce ExternalTypeRefInfoProvider #60497

augusto2112 · 2022-08-10T20:39:43Z

Introduce ExternalTypeRefInfoProvider

LLDB would like to cache typeref information to accelerate finding type
information. This patch adds an optional interface that allows of
typeref to register and provider field descriptor information for faster
lookups.

adrian-prantl · 2022-08-10T22:26:06Z

include/swift/Reflection/ReflectionContext.h

@@ -772,7 +772,7 @@ class ReflectionContext
  /// \return


comment is now out of date

include/swift/Remote/TypeInfoProvider.h

stdlib/public/Reflection/TypeRefBuilder.cpp

include/swift/Remote/TypeInfoProvider.h

augusto2112 · 2022-08-11T23:44:25Z

@swift-ci smoke test

mikeash · 2022-08-12T14:05:58Z

include/swift/Reflection/TypeRefBuilder.h

@@ -982,35 +991,46 @@ class TypeRefBuilder {
  IntVariableReader OpaqueIntVariableReader;

 public:
-  template<typename Runtime>


Looks like this is just a formatting change. Can we do that as a separate PR if so?

It got formatted because of the new remote::ExternalTypeRefInfoProvider *provider = nullptr parameter, but I guess it formatted the whole constructor 😅 I'll remove the non-related parts.

mikeash · 2022-08-12T14:06:57Z

Neat idea. Would it make sense to roll this logic into Remote Mirror rather than having it provided externally, or is it going to be very specific to LLDB in some way?

It would also be good to have tests to exercise the case when an external provider is given.

augusto2112 · 2022-08-12T16:31:31Z

Neat idea. Would it make sense to roll this logic into Remote Mirror rather than having it provided externally, or is it going to be very specific to LLDB in some way?

Could you explain what you're envisioning when you suggest rolling this logic into Remote Mirror? If it could benefit the runtime besides LLDB I'd be happy to do it. To give more context, LLDB's use case for this is to cache a map of mangled names to field descriptor locators (the field descriptor and the offset in the typeref buffer), and then provide them back in future runs. You can check the lldb patch here: swiftlang/llvm-project#5100. The interesting file to look at is SwiftMetadataCache.h and SwiftMetadataCache.cpp. If you see a way this could benefit the runtime as well we could change the approach.

augusto2112 · 2022-08-12T16:37:41Z

It would also be good to have tests to exercise the case when an external provider is given.

I'll try to come up with a way of testing this.

augusto2112 · 2022-08-12T16:38:06Z

swiftlang/llvm-project#5100

@swift-ci smoke test

mikeash · 2022-08-12T16:53:43Z

@augusto2112 and I discussed this some offline. Persistent caching wouldn't fit well on the Swift side. It might be feasible to put most of the caching here, with a get/set function that the client could use to persist it across runs. Then clients other than LLDB could use it pretty easily if they needed it. However, we don't have such a need yet, so we'll just keep that as a future possibility.

adrian-prantl · 2022-08-12T17:57:29Z

stdlib/public/Reflection/TypeRefBuilder.cpp

-          return *FD;
+      if ((!ExternalProvider ||
+           ExternalProvider->shouldParseReflectionInfoWithId(i)))
+        if (llvm::is_contained(ReflectionInfos[i].PotentialModuleNames,


I think this could use another comment explaining the algorithm and the contract with ExternalProvider here. I'm not sure it's obvious to the reader that !shouldParseReflectionInfoWithId(i) means we already parsed it.

adrian-prantl · 2022-08-12T17:58:39Z

stdlib/public/Reflection/TypeRefBuilder.cpp

-    if (auto FD = findFieldDescriptorAtIndex(i, *MangledName))
-      return *FD;
+    if ((!ExternalProvider ||
+         ExternalProvider->shouldParseReflectionInfoWithId(i)))


would

if (ExternalProvider && !ExternalProvider->shouldParseReflectionInfoWithId(i))) continuel

be easier to read?

"shouldParse" doesn't explain why. Should we name it "didCacheReflectionInfo(i)" or simething more descriptive?

I named the ExternalProvider in a deliberately not specific way just in case someone else wanted to implement the interface with a different strategy. I can change it to be more concrete though.

adrian-prantl · 2022-08-12T18:02:19Z

include/swift/Remote/ExternalTypeRefInfoProvider.h

+
+  /// Returns whether looking for a mangled name in reflection info with a given
+  /// id could possibly return information that couldn't be provided by the
+  /// external provider.


What would be an example?

If we have all of the field descriptors for that ReflectionInfo on the cache, and getFieldDescriptorLocator didn't find anything, then the field descriptor we're looking for must not be in that ReflectionInfo.

augusto2112 · 2022-08-15T17:38:42Z

swiftlang/llvm-project#5100

@swift-ci smoke test

augusto2112 · 2022-08-15T17:40:59Z

@adrian-prantl made the new interface closer to how it's actually implemented, which should make the logic easier to understand.

adrian-prantl · 2022-08-16T20:14:40Z

include/swift/Remote/ExternalTypeRefCacheProvider.h

@@ -0,0 +1,74 @@
+//===--- ExternalTypeRefCacheProvider.h - Abstract access to external caches of
+//typeref ------*- C++ -*-===//


The ExternalTypeInfoProvider is called so because it provides typeinfo records from the outside. Here, I think ExternalTypeRefCache alone would suffice (without the `provider)?

Thinking about it ... do we even need the External in the type name? I.e.: we could pass around a TypeRefCache *ExternalTypeRefCache

I agree with removing "provider", I do think "external" in the type name makes it more obvious what it is, versus as the variable name.

include/swift/Remote/ExternalTypeRefCacheProvider.h

stdlib/public/Reflection/TypeRefBuilder.cpp

augusto2112 · 2022-08-17T17:59:24Z

swiftlang/llvm-project#5100

@swift-ci smoke test

augusto2112 · 2022-08-17T18:29:10Z

swiftlang/llvm-project#5100

@swift-ci smoke test

augusto2112 · 2022-08-17T19:26:21Z

swiftlang/llvm-project#5100

@swift-ci smoke test Linux

include/swift/Reflection/ReflectionContext.h

adrian-prantl

LGTM with inline comments addressed

LLDB would like to cache typeref information to accelerate finding type information. This patch adds an optional interface that allows of typeref to register and provider field descriptor information for faster lookups.

augusto2112 · 2022-08-17T21:07:55Z

swiftlang/llvm-project#5100

@swift-ci smoke test Linux

augusto2112 · 2022-08-17T22:19:04Z

swiftlang/llvm-project#5100

@swift-ci smoke test macOS

augusto2112 requested review from kastiglione and adrian-prantl August 10, 2022 20:39

augusto2112 mentioned this pull request Aug 10, 2022

[lldb] Implement cache for swift typeref metadata swiftlang/llvm-project#5100

Merged

adrian-prantl reviewed Aug 10, 2022

View reviewed changes

include/swift/Remote/TypeInfoProvider.h Outdated Show resolved Hide resolved

augusto2112 force-pushed the swift-meta-cache branch 2 times, most recently from 59a2201 to 3ec6462 Compare August 11, 2022 23:43

augusto2112 changed the title ~~Register and look up field descriptors using TypeInfoProvider~~ Introduce ExternalTypeRefInfoProvider Aug 11, 2022

augusto2112 requested a review from mikeash August 11, 2022 23:44

augusto2112 marked this pull request as ready for review August 11, 2022 23:44

augusto2112 requested a review from jckarter August 11, 2022 23:44

mikeash reviewed Aug 12, 2022

View reviewed changes

augusto2112 force-pushed the swift-meta-cache branch from 3ec6462 to 4837a8e Compare August 12, 2022 16:37

adrian-prantl reviewed Aug 12, 2022

View reviewed changes

augusto2112 force-pushed the swift-meta-cache branch from 4837a8e to 6e1e053 Compare August 15, 2022 17:38

augusto2112 requested a review from adrian-prantl August 15, 2022 17:39

adrian-prantl reviewed Aug 16, 2022

View reviewed changes

include/swift/Remote/ExternalTypeRefCacheProvider.h Outdated Show resolved Hide resolved

adrian-prantl reviewed Aug 16, 2022

View reviewed changes

include/swift/Remote/ExternalTypeRefCacheProvider.h Outdated Show resolved Hide resolved

adrian-prantl reviewed Aug 16, 2022

View reviewed changes

include/swift/Remote/ExternalTypeRefCacheProvider.h Outdated Show resolved Hide resolved

adrian-prantl reviewed Aug 16, 2022

View reviewed changes

stdlib/public/Reflection/TypeRefBuilder.cpp Outdated Show resolved Hide resolved

augusto2112 force-pushed the swift-meta-cache branch from 6e1e053 to c9f2dc7 Compare August 17, 2022 17:59

augusto2112 changed the base branch from rebranch to main August 17, 2022 18:28

adrian-prantl reviewed Aug 17, 2022

View reviewed changes

include/swift/Reflection/ReflectionContext.h Outdated Show resolved Hide resolved

adrian-prantl approved these changes Aug 17, 2022

View reviewed changes

Introduce ExternalTypeRefCache

63a2c76

LLDB would like to cache typeref information to accelerate finding type information. This patch adds an optional interface that allows of typeref to register and provider field descriptor information for faster lookups.

augusto2112 force-pushed the swift-meta-cache branch from c9f2dc7 to 63a2c76 Compare August 17, 2022 21:07

augusto2112 merged commit 70d9f16 into swiftlang:main Aug 18, 2022

		@@ -0,0 +1,74 @@
		//===--- ExternalTypeRefCacheProvider.h - Abstract access to external caches of
		//typeref ------- C++ --===//

Introduce ExternalTypeRefInfoProvider #60497

Introduce ExternalTypeRefInfoProvider #60497

Uh oh!

Conversation

augusto2112 commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augusto2112 commented Aug 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeash commented Aug 12, 2022

Uh oh!

augusto2112 commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augusto2112 commented Aug 12, 2022

Uh oh!

augusto2112 commented Aug 12, 2022

Uh oh!

mikeash commented Aug 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

augusto2112 commented Aug 15, 2022

Uh oh!

augusto2112 commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augusto2112 commented Aug 17, 2022

Uh oh!

augusto2112 commented Aug 17, 2022

Uh oh!

augusto2112 commented Aug 17, 2022

Uh oh!

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

augusto2112 commented Aug 17, 2022

Uh oh!

augusto2112 commented Aug 17, 2022

Uh oh!

Uh oh!

augusto2112 commented Aug 10, 2022 •

edited

Loading

augusto2112 commented Aug 12, 2022 •

edited

Loading