[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6924a49690ee: [clang][modules] Account for non-affecting inputs in `ASTWriter` (authored by jansvoboda11). Changed prior to commit: https://review

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one suggestion for the test inline. Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:22 +// Build modules with the non-affecting "a/module

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 updated this revision to Diff 472466. jansvoboda11 marked an inline comment as done. jansvoboda11 added a comment. Rebase, decrease nesting, test using `diff` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136624/new/ https://reviews.ll

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900593 , @jansvoboda11 wrote: > Ah, I forgot to mention this. Building the modules is now only 0.2% slower > and importing them 1.2% faster (compared to PCMs with all input files > serialized). Awesome. All upsi

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 marked 4 inline comments as done. jansvoboda11 added a comment. In D136624#3900183 , @dexonsmith wrote: > Partly, trying to dig into why read speeds got slower. But maybe that was > noise that went away though when you switched to cycles/in

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900051 , @jansvoboda11 wrote: >> - Is there a change in cycles/instructions when the module cache is hot? >> (presumably the common case) > > I didn't notice this (but didn't look for it specifically). How could t

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 marked 5 inline comments as done. jansvoboda11 added inline comments. Comment at: clang/include/clang/Serialization/ASTWriter.h:449-452 + /// Exclusive prefix sum of the lengths of preceding non-affecting inputs. + std::vector NonAffectingInputOffsetAdjustments; +

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 added a comment. In D136624#3893607 , @dexonsmith wrote: > I'm curious; are system modules allowed to be non-affecting yet, or are they > still assumed to be affecting? (It's the system modules that I think are most > likely to be adjacent

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3893001 , @jansvoboda11 wrote: > I tried implementing your suggestion (merging ranges of adjacent > non-affecting files and avoiding `FileID` lookups), but the numbers from > `-ftime-trace` are very noisy. I got m

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 added a comment. I tried implementing your suggestion (merging ranges of adjacent non-affecting files and avoiding `FileID` lookups), but the numbers from `-ftime-trace` are very noisy. I got more stable data by measuring clock cycles and instruction counts, but nothing conclusive

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 updated this revision to Diff 471641. jansvoboda11 added a comment. Avoid looking up `FileID` for an offset. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136624/new/ https://reviews.llvm.org/D136624 Files: clang/include/clang/Basic

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added inline comments. Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32 + +// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o dexonsmi

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3888387 , @jansvoboda11 wrote: > I tried optimizing this patch a bit. Instead of creating compact data > structure and using binary search to find the preceding non-affecting file, I > now store the adjustment inf

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 updated this revision to Diff 471152. jansvoboda11 added a comment. New version with flat vector + `FileID` indices; replacing the previous compact representation & binary search approach Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 added a comment. I tried optimizing this patch a bit. Instead of creating compact data structure and using binary search to find the preceding non-affecting file, I now store the adjustment information for each `FileID` in a vector. During deserialization, `FileID` is simply used a

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 added a comment. In D136624#3880526 , @Bigcheese wrote: > This looks reasonable. Have you measured the performance impact of this > change? I have done comparison between this patch and https://github.com/apple/llvm-project/pull/5451 (that

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

Bigcheese added a comment. This looks reasonable. Have you measured the performance impact of this change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136624/new/ https://reviews.llvm.org/D136624 ___ c

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

jansvoboda11 created this revision. jansvoboda11 added reviewers: dexonsmith, Bigcheese, vsapsai, ilyakuteev. Herald added subscribers: ributzka, mgrang. Herald added a project: All. jansvoboda11 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commit