Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Excessively high compile times #86

Open
Lazrius opened this issue Apr 11, 2024 · 12 comments
Open

Excessively high compile times #86

Lazrius opened this issue Apr 11, 2024 · 12 comments

Comments

@Lazrius
Copy link
Contributor

Lazrius commented Apr 11, 2024

Hello again. My coworkers and I have observed a massive increase in compile times ever since we have implemented your library into our project. To make the project compile, we've had to add the following compiler flag: clang:-fconstexpr-depth=2048, which strongly implies there is a heavy amount of recursion at play.

As it stands, we are currently only reflecting two structs. One of them looks as follows:

struct Character
{
        std::optional<bson_oid_t> _id;
        std::string accountId;
        std::string characterName;
        int money = 0;
        int rank = 0;
        int64 affiliation = 0;
        std::optional<std::string> repGroup;
        std::optional<Vector> pos;
        std::optional<Vector> rot;
        std::string voice;
        int interfaceState = 0;
        float hullStatus = 1.f;
        float baseHullStatus = 1.f;
        bool canDock = true;
        bool canTradeLane = true;
        int64 lastDockedBase = 0;
	int64 currentBase = 0;
        int64 currentRoom = 0;
        int killCount = 0;
        int missionFailureCount = 0;
        int missionSuccessCount = 0;
        int64 shipHash = 0;
        int64 system = 0;
        int64 totalTimePlayed = 0;
        Costume baseCostume;
        Costume commCostume;
        std::vector<FLCargo> cargo;
        std::vector<FLCargo> baseCargo;
        std::vector<Equipment> equipment;
        std::vector<Equipment> baseEquipment;
        std::unordered_map<int, float> collisionGroups;
	std::unordered_map<int, float> baseCollisionGroups;
        //keys are actually unsigned ints, but this works anyway
        std::unordered_map<int, float> reputation;
        std::unordered_map<int, int> shipTypesKilled;
        std::unordered_map<int, int> randomMissionsCompleted;
        std::unordered_map<int, int> randomMissionsAborted;
        std::unordered_map<int, int> randomMissionsFailed;
        std::unordered_map<int, char> visits;
        std::vector<int64> systemsVisited;
        std::vector<int64> basesVisited;
        std::vector<NpcVisit> npcVisits;
        std::vector<int64> jumpHolesVisited;
        std::unordered_map<int, std::vector<std::string>> weaponGroups;
};

We have no plans to introduce structs larger than this one, but the very notable increase in compile time (from about 30-40 seconds to around 5:30 minutes) completely cripples our ability for rapid iteration, slowing down development of several critical features to a standstill.

Thanks again for the great work, and I know it's not an easy thing to diagnose, but we'd greatly appreciate any support in getting those compile times to a more manageable level.

@liuzicheng1987
Copy link
Contributor

This one is very tricky...the library uses quite a bit of templating which has been known to increase compile time.

But I am curious about clang:-fconstexpr-depth=2048. I don't quite understand why this is necessary, even for a struct your size. I will have to take a closer look.

@liuzicheng1987
Copy link
Contributor

liuzicheng1987 commented Apr 12, 2024

One thing you could do to alleviate the rapid iteration problem is to outsource the parsing into a separate compilation unit.

Basically, have on .cpp file that just contains rfl::bson::read.

If you are using something like cmake and ccache, I don't imagine you would have to recompile the parsing of your two structs unless you actually change anything about the structs.

At the end of the day, it is important to remember that all of the boilerplate code that is abstracted away by the library still needs to be generated and compiled. So something like rfl::json::read is a one-liner to you but a lot is happening under the hood

@liuzicheng1987
Copy link
Contributor

I have been thinking about this...I am fairly sure that the reason you need recursion to be that deep is because of C arrays, more specifically the things we are forced to do to support them.

I think one thing we could try is is to turn off support for them using a compiler flag (which I would have to implement). You could then just use std::array instead.

@Lazrius
Copy link
Contributor Author

Lazrius commented Apr 14, 2024

I was not aware of CCache, and will try and implement that. The code for serializing is currently located with other related code, but I think we can move it out to its own file somewhere.

The compiler flag sounds like a good option!

@liuzicheng1987
Copy link
Contributor

@Lazrius, the easiest way to get the parsing into a separate compilation unit is to use custom constructors:

https://github.com/getml/reflect-cpp/blob/main/docs/bson.md

@liuzicheng1987
Copy link
Contributor

@Lazrius , I have added experimental support for a flag that might at least get rid of the recursion problem:

https://github.com/getml/reflect-cpp/tree/f/build_time_performance

You can add -DREFLECT_CPP_NO_C_ARRAYS_OR_INHERITANCE to your compile flags. Assuming that your structs do not involve inheritance or contain C arrays (std::array is fine), this would probably get rid of the recursion problem and you won't have to add clang:-fconstexpr-depth=2048.

@Lazrius
Copy link
Contributor Author

Lazrius commented Apr 26, 2024

Very awesome. I will see if I have time to try this on the weekend, I should be free on Sunday to give it a go.

@Aingar
Copy link

Aingar commented May 5, 2024

Hey, I'm working with Lazrius on this issue, and we have tried the REFLECT_CPP_NO_C_ARRAYS_OR_INHERITANCE flag, but for reasons we cannot comprehend, it breaks on a very basic struct. It works just fine if both instances of CostumeData are commented out. The original Costume struct is exactly the same except for the array being a C-array.

ObjectsAndErrors.txt

@suy
Copy link

suy commented May 13, 2024

My 2 cents: I've started to use the library, and the quick experiment went fine, but when applying it to a "real" class which has a good amount of members, the compilation time went to really bad. A file which normally compiles in 2 or 3 seconds, takes 3 minutes instead. :-(

I'm just starting to use the feature adding in #59, nothing more. I just want to loop over the fields of some structs.

I tried to pinpoint if there was a serious issue with some of the types I'm using (it's mostly integral types), and if QString being one was an issue, but it seems what makes it slow compilation a lot is the number of fields. I also had to pass -fconstexpr-depth=1024, as otherwise it was failing to compile.

Additionally, I tried both master (6bb95ff) and the f/build_time_performance branch.

Thank you very much for your effort! This library is very exciting and promising.

@Lazrius
Copy link
Contributor Author

Lazrius commented May 14, 2024

Some additional information. We tried changing up which compiler we were using to see what effect it would have. Compiling on Windows using MSVC, our entire project which makes heavy use of templates, was 1m 15s at last check. It would seem that most of the time on that build wasn't even within the reflection code either.

On the other side, when compiling MSVC using the ClangCL toolchain, the compile time was over 5 minutes, and the vast majority of time seemed to be spent within the reflection library.

I am curious if it is things like compiler checks, of which clang seems to be far more aggressive with, is partially responsible.

@liuzicheng1987
Copy link
Contributor

@suy , can you provide some examples of what it is that you are doing there?

A problem that I often see is that a one-liner like rfl::json::read<MyObject>(...) entices people to just put that into their header files. That's a bit of a problem, because A LOT is happening here under-the-hood and it forces you to re-compile the complex serialization code for every compilation unit. So it would be a good idea to put that into its own compilation unit (see my comments above).

But I would be very interested to see some reproducible code examples here.

@liuzicheng1987
Copy link
Contributor

liuzicheng1987 commented May 14, 2024

@Lazrius , thanks for the information. It's very weird to see that Clang should be the slowest compiler, because my experience is exactly the opposite. Just take a look at our Github Runners.

By the way, we are working on this topic. It's something that affects us as well.

liuzicheng1987 added a commit that referenced this issue Jul 3, 2024

---------

Co-authored-by: Patrick Urbanke <patrick@getml.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants