[PATCH] D109632: [clang] de-duplicate methods from AST files

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D109632#3012647 <https://reviews.llvm.org/D109632#3012647>, @rmaz wrote:

>> What folks are thinking about writing less in METHOD_POOL?
>
> I prefer the idea of it, but I think the `ReadMethodPoolVisitor` also has to be changed for this to work. When it finds a selector in a module it will return true, which causes the search to stop descending into dependent modules:
>
>   if (!Visitor(*CurrentModule))
>     continue;
>   
>   // The visitor has requested that cut off visitation of any
>   // module that the current module depends on. To indicate this
>   // behavior, we mark all of the reachable modules as having been visited.
>
> Wouldn't this logic have to be changed to ensure we pick up all the transitive methods from dependent modules?

You are right, suggested implementation is insufficient and I've added a test to https://reviews.llvm.org/D110123 to catch it.

Right now `MODULE_POOL` seems to be caching all transitive modules but it does so poorly in case of shared dependencies and sometimes the cache is empty forcing you to collect methods from imported modules. So it looks like we are somewhere in the middle on caching --- non-caching spectrum. I'd prefer to go entirely to one of the ends and for that I see 2 options:

1. Deduplicate methods from shared dependencies when reading method pools from imported modules.
2. Serialize only methods owned by the current module (and change `ReadMethodPoolVisitor` appropriately).

I think the option 2 would be better as it seems to be easier to understand. Implementation complexity seems to be comparable, runtime seems to be better for option 2. Also given that collecting methods from a module is basically `InstanceMethods.append(Data.Instance.begin(), Data.Instance.end())`, I don't think caching methods from transitive dependencies saves us processing time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632