[PATCH] D36352: [LCG] Switch one of the update methods for the LazyCallGraph to support limited batch updates.

[Sean Silva via Phabricator via llvm-commits]

silvas accepted this revision.
silvas added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D36352#833455, @chandlerc wrote:

> Really nice analysis. I hadn't gotten that far, so that is helpful. Sadly, the descriptor stuff being the source isn't too surprising to me.
>
> Anyways, any thoughts about the patch itself? I've got at least one more fix prepared behind this.


Overall it looks fairly mechanical and slightly cleaner after the patch. LGTM.

Also, I looked a bit more into the graph I posted above. There is one ref edge which is what really causes the large RefSCC: `_ZN12_GLOBAL__N_130protobuf_AssignDescriptorsOnceEv` -> `_Z31protobuf_AssignDesc_foo_2eprotov`. That edge comes from the once initialization code

  GOOGLE_PROTOBUF_DECLARE_ONCE(protobuf_AssignDescriptors_once_);                                                                                                                                             
  inline void protobuf_AssignDescriptorsOnce() {
    ::google::protobuf::GoogleOnceInit(&protobuf_AssignDescriptors_once_,
                   &protobuf_AssignDesc_foo_2eproto);
  }

(it's a ref edge, but if we were to fully inline through GoogleOnceInit it would become a call edge. The actual dispatch to protobuf_AssignDesc_foo_2eproto happens through an external function though so we're saved)

If that edge is deleted (which we can't do statically, but just for the sake of investigation), then these are the only remaining nontrivial RefSCC's.

F4829448: Screenshot from 2017-08-07 11-00-13.png <https://reviews.llvm.org/F4829448>
https://reviews.llvm.org/F4829448

There is one nontrivial RefSCC of constant size per message (Foo, Bar, Message3) where all edges within the RefSCC are ref edges. These are just all the functions that statically reference the vtable (constructor/destructor), together with any virtual functions that call back into them. These are basically constant size independent of the number of messages.

Then there is one RefSCC that is O(number of messages in the .proto file). The `protobuf_AddDesc_foo_2eproto` RefSCC. Note that all edges here are call edges. Basically this routine just recursively default intializes all default instances, which themselves recursively initialize any instances they depend on. The thing that ties the SCC together is that default_instance() looks like this:

  const Baz& Baz::default_instance() {
    if (default_instance_ == NULL) protobuf_AddDesc_foo_2eproto();
    return *default_instance_;
  }

However, only the default instance stuff gets pulled into this SCC, so even though it is O(number of messages in the proto file) the constant is much smaller than when the edge `_ZN12_GLOBAL__N_130protobuf_AssignDescriptorsOnceEv` -> `_Z31protobuf_AssignDesc_foo_2eprotov` is present (which pulls in the MergeFrom and MergePartialFromCodedStream and a bunch of getters/setters and stuff into the RefSCC).



================
Comment at: lib/Analysis/LazyCallGraph.cpp:1133-1142
+  // Collect the SCCs for the source and targets.
   SCC &SourceC = *G->lookupSCC(SourceN);
-  SCC &TargetC = *G->lookupSCC(TargetN);
-  if (&SourceC == &TargetC)
+  SmallSetVector<SCC *, 4> TargetCs;
+  for (Node *TargetN : TargetNs)
+    TargetCs.insert(G->lookupSCC(*TargetN));
+
+  // If all targets are in the same SCC as the source, because no call edges
----------------
I found this a bit confusing. SourceC and TargetCs seem to be only used for this bailout here. So the section-delineating comment `// Collect the SCCs for the source and targets.` is a bit misleading (seems like it is collecting them for use later). Maybe just move the `If all targets are in the same SCC as the source ...` comment up to there and move the `if` a bit closer to make that clear.

In another patch you may want to consider putting the tarjan walk below into a separate function which I think would make this easier to understand. The main reason I think it seems pretty important to do that is to make it clear to the reader that we aren't doing anything fancy using our knowledge of the SourceC and TargetCs and are just walking everything in the RefSCC again, which makes the complexity obvious. (maybe beefing up the comments would help too)

(then again,  I can see why you'd want to keep it all inline, because above you raw delete edges and so some of the invariants might be broken until we finish the update.)


https://reviews.llvm.org/D36352