ChuanqiXu9 wrote: > Thanks for the change! I have done a round of review and left a few > suggestion and also have a bunch of questions. I am sorry if some of them may > look too obvious, I did try to dig into the code where I could, but Clang's > serialization is complicated and some things are easier researched through a > conversation than through looking at code. Please bear with me, I promise to > get better in the upcoming reviews.
You're welcome. > > Here are a few extra question that came to mind too. > > Question 1: Do we have estimates on how much bigger the PCMs become? Did you > observer any negative performance impact? > > I would expect `TypeID`s to be much more common than decls, and the > corresponding size increases to be more significant. We've already lost ~6% > from DeclID change, I am slightly worried the types are going to be a bigger > hit. I did and my local results shows, I see less than 1% change with this patch. This fits my understanding somehow. Since the `TypeID` is much less common than `DeclID`. This matches my experience in coding. The `DeclID` patch is the hardest one and the `TypeID` patch is the easiest one. > > Question 2: could you explain why we the PCM in your example was changing > before? Was there some base offset inherited from the PCM files we depended > on? Yes. In the higher level, it should be easy to understand. There is a new type in `m-partA.v1.cppm`. And in the low level, when we write a module file, we would record the type offset for the module files we depend on: https://github.com/llvm/llvm-project/blob/ad79a14c9e5ec4a369eed4adf567c22cc029863f/clang/lib/Serialization/ASTWriter.cpp#L5454 And this is exactly what the series patch want to eliminate. https://github.com/llvm/llvm-project/pull/92511 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
