WEBVTT 00:00.000 --> 00:12.800 All right, so hello everyone. I'm a done. I work on system D and I spent about a 00:12.800 --> 00:17.480 month or so last year cleaning up includes in system D. So I'm here to talk about that a 00:17.480 --> 00:22.720 little and hopefully it might be useful for the people in the room here as well. 00:22.720 --> 00:29.280 So let's start with why this matters. These are like some stats from after I did the work. 00:29.280 --> 00:37.040 What it basically came down to was that I got like a 32% increase or decrease luckily in 00:37.040 --> 00:47.280 a compilation times for a from scratch build and subCI jobs became faster by 50% just by removing 00:47.280 --> 00:53.280 includes from system D source and had a files that were unused. So how did I get started 00:53.280 --> 00:57.960 on this project? Well, I was just working on system D at some point and I use Clang D as my 00:57.960 --> 01:05.480 LSB and VS code and it's just crowned to a halt. Seemingly randomly as long as I had the 01:05.480 --> 01:11.320 project open for long enough and so without doing any like proper investigation I decided for 01:11.320 --> 01:16.520 myself this must be because we include to many headers. So I did a bunch of work to fix all the 01:16.520 --> 01:21.320 unused includes and then I figured out the issue was something totally different but well we still 01:21.320 --> 01:27.480 clean up the includes right so mission achieved. And so I ended up I did a small I don't know 01:27.480 --> 01:34.120 calculations not entirely correct but I ended up removing at least or at least yes more than 01:34.120 --> 01:41.640 3000 includes from all of system D source files. And then the question is why how do I how did I go 01:41.640 --> 01:49.000 about it? So you want to do two things you want to remove like unused includes and so this is mostly 01:49.000 --> 01:53.960 about source files where you just want to remove every include that doesn't provide a symbol or 01:54.120 --> 01:59.720 a definition or whatever that isn't being used but then you also want to remove or minimize the number 01:59.720 --> 02:05.480 of transient includes and this is more about header files that are included by other header files. So 02:07.480 --> 02:12.120 the way you do this usually is with forward declarations it's with moving macro implementation 02:12.120 --> 02:17.480 details into the source file and by minimizing the amount of online functions. So forward declarations 02:17.480 --> 02:22.680 mean it's easy or you don't have to include the full header file providing the symbol macro 02:22.680 --> 02:29.160 implementation means that if you have like I have to like add that like we don't write 02:29.160 --> 02:33.560 C++ right so we need macros for a lot of stuff so that's why we focus on macros a little bit as 02:33.560 --> 02:40.040 well and so like macros are basically a full function and because they need to be the final 02:40.040 --> 02:45.560 header file every include for to implement that macro needs to be an header file which pulls 02:45.560 --> 02:51.480 in a bunch of stuff which slows down compilation and then the online functions we had quite a 02:51.480 --> 02:55.480 few of those as well and depending on what they do they also need to pull in an extra header 02:55.480 --> 02:59.960 which is ways full if you move it to the source file and rely on altiodity optimization you 02:59.960 --> 03:05.640 can avoid the include and you again reduce the amount of the includes that are required to be included 03:05.640 --> 03:14.040 by the source files. So the first thing I did was actually fixing all the circular inclusions 03:14.040 --> 03:19.960 in this so we did this quite a bit across statements so the what would you would think of like 03:19.960 --> 03:24.440 you had a header file it had a four bunch of four declarations for the structures was going to 03:24.440 --> 03:28.760 declare later then the debuts of includes and then it actually defined the includes like this 03:28.760 --> 03:34.120 was this was absolutely horrifying when you would notice it like nobody would write same C++ 03:34.120 --> 03:41.160 this way these T's days but we did it a bit all over the place so I basically got rid of all of 03:41.160 --> 03:46.760 that and the the clank tidy check I used for this is missk header includes cycle so I think this one 03:46.760 --> 03:51.240 is like absolutely amazing to be able to have every project and just get rid of all the circular 03:51.240 --> 03:55.320 includes there's always a way to get rid of them and then I'll make my sense to have in the first place 03:55.320 --> 04:03.080 so I would recommend everyone to implement that one and I have it enabled by default and I also had 04:03.080 --> 04:08.280 a lot of issues with clank decrashing for seemingly unknown reasons and removing all the circular 04:08.920 --> 04:12.840 fixed that so I'm not sure what bug there was there but that also helped with that as well 04:13.800 --> 04:19.720 this was all like done by hand basically enabling the check running and then figuring out how 04:19.720 --> 04:26.200 I could best fix the circular includes you need to think through your headers or your source 04:26.200 --> 04:30.920 file and header file layout a little bit because you might need to move strokes around to be 04:30.920 --> 04:35.400 to avoid circular includes in the first place so it can get gnarly but I think it's almost always 04:35.560 --> 04:42.120 worth it to fix these and then we had to integrate clank tidy in the build system so we use 04:42.120 --> 04:51.240 mess on which has integration and as it's clank tidy target see make has it a similar thing where 04:52.280 --> 04:58.280 if you configure it it will just run clank tidy every time you compile the relevant source file 04:58.280 --> 05:03.400 and so I tried using this but there are a bunch of issues it doesn't really you can't 05:03.400 --> 05:07.960 add dependencies to the clank tidy target on sources that are generated by your build system 05:07.960 --> 05:14.600 which means that it will fail to run on those it runs on all source files regardless of whether 05:14.600 --> 05:18.200 their build or not and when you have a lot of conditional compilation and a project like 05:18.200 --> 05:23.400 system either also brings issues and you can't configure the target so that didn't really 05:23.400 --> 05:29.560 run great all of the m is a run clank tidy script I try that as well but it introduces its own form 05:29.560 --> 05:36.440 of scheduling right it runs parallel jobs and everything itself but we don't need that we already 05:36.440 --> 05:41.560 have mess on this build system it can run parallel jobs it can run tests and parallel and so that's 05:41.560 --> 05:47.640 what we ended up doing in the end we instead of like running clank tidy as part of the build we 05:47.640 --> 05:54.280 run it as a unit as and this has various benefits the main thing being that as soon as we get 05:54.280 --> 06:00.680 into the actual unused headers depending on the build configuration you're going to have that 06:00.680 --> 06:05.560 clank tidy check trigger more often or not and on different headers if you're building without 06:05.560 --> 06:12.360 some dependency or anything different right you can all of those build system configurations can 06:12.360 --> 06:20.040 end up giving you more unused headers so getting your entire build configurations like every 06:20.040 --> 06:24.920 single combination of build system option like library enabled or not right this library might 06:24.920 --> 06:28.760 be enabled that order library might be disabled or anything else can give you very 06:28.760 --> 06:34.120 different results so if you enable this on build right if you do grunk clank tidy every time 06:34.120 --> 06:38.840 you use a compiler source file it's just impossible to get working so I find it much easier 06:38.840 --> 06:44.440 as a unit test so that you can put it in its own suite and only run it for very specific build 06:44.440 --> 06:53.080 configurations the first thing I thought it was actually IWU so include what you use which is 06:53.080 --> 06:59.720 a project by Google which uses the clank c++ API and then you immediately run into the issue 06:59.720 --> 07:05.320 that clank c++ API does not offer backwards compatibility and so like immediately it was already 07:05.320 --> 07:10.760 depending on features from clank in get main whereas my distribution was only shipping the 07:10.760 --> 07:17.880 stable version it wouldn't even compile so that was kind of annoying it also had a bunch of 07:17.880 --> 07:23.320 missing features like we use the clean up attributes from gcc and well clank as well in system 07:23.320 --> 07:29.720 need to have the structures just like c++ that wasn't supported so for the clean up attribute 07:29.720 --> 07:34.600 you pass it a function and that function was not detected so it would mark a bunch of stuff as 07:34.600 --> 07:39.800 unused even though it was being used in the clean up attribute I send a PR for that I fixed some 07:39.800 --> 07:45.640 other stuff as well but in the end I didn't end up using it because of the compatibility issues 07:45.640 --> 07:53.160 with the clank c++ API and because the clank thing ended up being much nicer to use so the clank 07:53.160 --> 07:59.480 thing is clank include clean so this was a project that originated in clank d the language server 07:59.480 --> 08:04.920 but then they decided to pull it out into its own tool which is clank include clean it's less 08:05.480 --> 08:13.080 opinionated right what I will you I will you I will do is it will report like it will remove 08:13.080 --> 08:19.640 an used includes but it will also always add missing includes so it insists on directly including 08:19.640 --> 08:25.160 the header providing a symbol for everything a source file so it doesn't like if you have transitive 08:25.160 --> 08:33.480 includes it will still add all the transient sorry transitively included includes into the 08:33.480 --> 08:39.400 source file so you end up with huge changes which I mean it's arguable whether that model is 08:39.400 --> 08:44.440 useful enough but I wanted to keep the changes somewhat minimal and clank included it easily 08:44.440 --> 08:49.480 allowed me to only focus on removing unused includes and keeping the transient includes in place 08:49.480 --> 08:56.920 as they are except with all VM which just makes it much easier to use interestingly enough 08:56.920 --> 09:01.960 and that exactly the same bug as I will you or you which is the cleaner attribute also wasn't 09:01.960 --> 09:08.360 passed fixing it in clank include cleaner was a bit more hairy than fixing it in include what you 09:08.360 --> 09:14.920 use but I ended up figuring it out eventually I will include what use was also much easier to compile 09:14.920 --> 09:21.000 in LVM or I had to spend hours waiting for it to build it finish but eventually I figured it out 09:21.320 --> 09:26.840 and then the the best thing is that it is integrated into clank tidy so instead of having to 09:26.840 --> 09:32.040 figure out the built integration for that in system D right like or invoking a separate tool 09:32.040 --> 09:36.520 by the built system after thing no I just had to enable a single check in clank tidy and I got 09:36.520 --> 09:46.920 all my unused includes reported file unit as failures so then you get to how to manage the source 09:46.920 --> 09:54.360 code to make this somewhat like viable like I said one of the solutions to solving or to removing 09:54.360 --> 10:01.640 transient includes in header files is to use forward declarations instead but repeating the same 10:01.640 --> 10:06.840 forward declarations in every single file does not scale like it ends up being incredibly for posts 10:06.840 --> 10:11.160 so the solution we found there was simply like forward declaration errors I mean we didn't find 10:11.160 --> 10:16.440 the solution I think the standard library even does is itself so we ended up adopting that as well 10:16.920 --> 10:23.000 we simply have like a few files like we call them forward or age and with a prefix and that's 10:23.000 --> 10:29.080 just simply all the forward declarations we can imagine right so all the structs all the even all 10:29.080 --> 10:34.600 the enums reformer forward declare though we can forward declare their size yet I think that's 10:34.600 --> 10:40.440 coming in C 2020 or 23 somewhere once we can do that we can get rid of even more headers 10:40.440 --> 10:45.960 because we can forward declare the size and when we want to have to include the header file declaring 10:46.040 --> 10:52.840 the enum anymore and then to make things easy to use we also made those forward dot h files include 10:52.840 --> 11:00.520 the most commonly included header files for C and so like standard n dot h types dot h like there's a 11:00.520 --> 11:05.640 bunch of stuff error node dot h which you are always going to end up using so it doesn't make 11:05.640 --> 11:09.560 sense to repeat the include for every single source file you just put it in one of those forward 11:09.560 --> 11:15.400 dot h letters and you get it for free by just including that single file and you're not compiling 11:15.480 --> 11:21.080 more than you are parsing more than you did before because it was going to get include files 11:21.080 --> 11:28.040 on trends and include anyway um so then there is a conditional compilation like I said like 11:28.040 --> 11:32.520 depending on what you're enable the uh the uh just includes are going to be different and so instead 11:32.520 --> 11:36.840 of trying to tackle that problem and making sure every single build configuration works 11:38.520 --> 11:43.720 what I ended up doing is to just define a very specific build configuration in which we run 11:43.800 --> 11:49.000 clank tidy and that's the one where we that that's the one that runs the CI and the one that we 11:50.200 --> 11:55.480 that we like officially support and that configuration is just the one where we enable every single 11:55.480 --> 12:02.440 feature we have so that we get the most coverage for all the different libraries and stuff that we 12:02.440 --> 12:07.800 enable of course this doesn't work when it's enabled or not but if you have to make a choice 12:07.800 --> 12:12.280 between one library or the order which we do have in a few cases and you have to make a choice 12:12.680 --> 12:18.040 and for those cases I did end up making sure that both configurations work so that means 12:18.040 --> 12:23.160 like putting a bunch of includes behind if that's to make sure they're not unused when the 12:23.160 --> 12:30.280 other library is used which is kind of painful to do but it works and then the next problem 12:30.280 --> 12:36.440 is system matters so um include why you use an clank and include cleaner have a bunch of 12:37.240 --> 12:46.840 pragmas like what they call it to instruct the thing to do how do you say it like it's not a 12:46.840 --> 12:52.840 foolproof algorithm right so sometimes for example when you're transitive when you want to include 12:52.840 --> 12:58.520 one header you always want to have it exports the symbols from another file you need to use these 12:58.520 --> 13:04.840 pragmas to say okay this header file exports this other header file and stuff like that so I mean 13:04.920 --> 13:10.920 use that to like make the thing work basically the problem is you cannot you can only add these 13:10.920 --> 13:14.920 pragmas to your own header files right you cannot go and modify your system libraries to have 13:14.920 --> 13:19.560 these as well include what you use had a solution for this file configuration file where you 13:19.560 --> 13:25.480 could add these pragmas but clank and include it doesn't really support that so it does luckily have 13:25.480 --> 13:31.560 like a rag eggs mechanism to exclude a bunch of headers from getting reported as unused so we use 13:31.640 --> 13:37.000 that to get around it and like the most annoying example I ran into was get up to age 13:37.000 --> 13:42.360 and this is just a header file that does nothing but include a bunch of other header files so 13:42.360 --> 13:47.240 it would always get marked as unused because it doesn't export any symbols and I cannot go and 13:47.240 --> 13:51.720 fix it to add those pragmas so I have to exclude it that's slightly annoying I would prefer 13:51.720 --> 13:56.840 that there was like some way to configure clank and clook cleaner that I could say that get up to 13:56.840 --> 14:01.080 the age exports these headers and it would just work but that's not supported so I have to deal with it 14:05.960 --> 14:10.120 it took me about I don't know I like it took me about a month to get all the PRS ready and merge 14:11.160 --> 14:17.080 for removing all the in a new thing includes but then last month when I was preparing the stock 14:17.080 --> 14:21.000 I also thought okay I should enable some more clank tidy tanks and so there are a bunch of 14:21.080 --> 14:27.480 useful ones the argument comment like system C++ and C don't support names arguments so 14:27.480 --> 14:33.800 what you end up doing is you end up putting an inline comment with the argument name and what clank 14:33.800 --> 14:41.000 tidy can do is it can tell you if that comments mismatches with the actual declaration in the header 14:41.560 --> 14:47.400 and because over time you reflect the code you rename arguments but you don't get a compilation 14:47.400 --> 14:52.280 error for those for those comments right and clank tidy can like if you're an error when those things 14:52.280 --> 14:58.520 start to mismatch and so I found like thousands of wrongly named argument comments which I all fixed one 14:58.520 --> 15:04.600 by one so that there are no fixed and you get a CIR this time when you when you reflect our function 15:04.600 --> 15:10.120 argument name and you don't update the comments readability and consistent declaration parameter 15:10.120 --> 15:17.080 name is the same thing but for mismatches between a function declaration and it's header so the 15:17.080 --> 15:23.080 argument names can also start to mismatch between declaration and it's definition also thousands 15:23.080 --> 15:27.640 and thousands of errors insist indeed what that where we renamed it in the definition but on 15:27.640 --> 15:32.360 the declarations are also when the fixed all of those so those who are also really useful to detect 15:32.360 --> 15:38.760 all of those and to also to prevent new regressions that's the most important thing and then the 15:38.760 --> 15:45.640 size of one was also pretty useful. The final one that we want to enable but can't 15:45.640 --> 15:52.600 slang format so a system D is very particular about its formatting my fellow maintainers refuse 15:52.600 --> 15:58.920 to budge right so I can't change the style I have to fix the tool so I'm slowly trying to fix 15:59.240 --> 16:05.400 format to support our our system D formatting style what I'm currently trying to add is we 16:06.840 --> 16:12.120 in the return types and in the cast the C-style cast we align the pointer to the left instead of the 16:12.120 --> 16:19.960 right and other the clang format one forces you to all of them to the right of all of them to the 16:19.960 --> 16:24.120 left so I need to make it configurable like it's a bit of a pain I'm slowly working through it but I 16:24.120 --> 16:29.320 hope that like maybe in a year or two we'll be able to enable clang format for system D so I don't 16:29.320 --> 16:39.320 have to manually format my code anymore. Yeah yeah they don't they don't like me adding these features 16:39.320 --> 16:42.760 to clang format either I get a lot of pushback but like I'm stuck with between however I 16:42.760 --> 16:49.160 hammer an envelope or something and then there was also a bunch of bugs in clang format that I still 16:49.160 --> 16:54.520 need to fix but hopefully I'll get time to that once I will finish waiting for the build to finish 16:56.120 --> 17:00.120 that was it I'll put answer questions 17:05.800 --> 17:11.800 if I'm not wrong you can run clang type if I clang like in a white confine so you didn't get 17:11.800 --> 17:19.080 it because for me I have some very experienced with that but I invite you to try this clang clang 17:19.080 --> 17:24.600 tiny clang yeah so the remark was that there's a plug-in that you can run clang tally as a clang 17:24.600 --> 17:31.000 plug-in so that it runs while it's compiling the code so that's basically what see make ends 17:31.000 --> 17:36.840 of doing in its own implementation but I think like I wouldn't need to like I mean I wouldn't 17:36.920 --> 17:42.760 need to toggle to add a toggle to only run it in various specific cases so that because most 17:42.760 --> 17:46.280 of the build configurations you would run clang tally and it's probably not gonna it's gonna 17:46.280 --> 17:51.560 report unused includes but it would be an option but I I don't know I might still prefer 17:51.560 --> 17:56.520 the unit test because you might want to rerun it a few times and if the file is already compiled 17:56.520 --> 18:01.880 and incremental compilation will prevent you from running clang tally again whereas that's always 18:02.520 --> 18:10.680 anoint me about that approach well one more question 18:21.160 --> 18:28.600 I use so the question is because this is a very large code based like how did I approach 18:28.600 --> 18:35.800 like subdividing the problem so the circular includes I just started I mean we we 18:35.800 --> 18:41.880 component or component like you don't there's no proper way to split that up there but then when I 18:41.880 --> 18:48.600 actually and then removing transient in or sorry transdiving includes you start at the tops 18:48.600 --> 18:53.960 of the dependency tree and then you move down and then what so but that was solely for the headers 18:54.040 --> 19:00.920 but then when you start removing unused includes from the how do you say it from the actual 19:00.920 --> 19:06.760 like implementation files and I mean so because removing transdiving includes it's the hard part 19:06.760 --> 19:10.920 right you have to rewrite code you have to move function definitions around and stuff so 19:10.920 --> 19:16.120 that's why I started with them solely for removing unused includes that's where I started 19:16.840 --> 19:21.400 at the bottom of the tree so you start with actual damings and binaries that aren't libraries 19:21.480 --> 19:26.040 so aren't depending on dependent on by anything else so those are easy because they only touch 19:26.040 --> 19:31.080 themselves and then you move on to the libraries where you if you were moving unused include 19:31.080 --> 19:35.880 from header there you don't just affect the library you also affect every consumer of the library 19:35.880 --> 19:41.480 so that's how I handled it I think that's what I mean you can get going easily by doing the 19:41.480 --> 19:46.520 damings because you the the piars are small but then when you get to the libraries you end up with 19:46.520 --> 19:52.600 massive piars because you have to remove remove and add includes all over the place so that's how I handled that 20:03.480 --> 20:10.040 yes so the question was did you notice any performance impact from moving inline functions to 20:10.680 --> 20:17.240 a lot non-line functions by moving them to the implementation file and so no but I have to 20:17.240 --> 20:25.880 add that our performance monitoring and system D is effectively non-existent so like it would have 20:25.880 --> 20:30.120 been a user reported issue but we didn't get any user regression support of our performance but 20:30.120 --> 20:34.520 like we were running blind effectively so there might have been something 20:34.520 --> 20:41.800 that's it I think thank you