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

Slow compile times => alternative organization of source #445

Closed
aparajita opened this issue Jan 2, 2017 · 17 comments
Closed

Slow compile times => alternative organization of source #445

aparajita opened this issue Jan 2, 2017 · 17 comments

Comments

@aparajita
Copy link
Contributor

I love this lib, but it doubles my compile times. It turns out that there is a ton of non-templated, non-trivial code in the header which is slowing down compiles.

I made a branch that moves all non-templated, non-trivial code to a source file which is added to my projects, and voilà! Compile times are back to normal.

Before I take the time to submit a pull request, I'm wondering if you (and other users) are interested at all in making this change.

@abumq
Copy link
Owner

abumq commented Jan 2, 2017

Hi and thanks for the interest in this library.

Yes sure but can you put both header and source file in seperate dir under src/with-source-file (or any better alternative)
I'm thinking i will release both header-only and this version

Also please rebase it from develop branch and push it back to develop branch (in PR)

@aparajita
Copy link
Contributor Author

If there is going to be a with-source-file directory, then I think there should also be header-only directory for the single header version, just to be consistent and clear.

@aparajita
Copy link
Contributor Author

You'll quickly grow tired of applying changes to two versions of the same code spread across three files.

So instead of manually maintaining two versions, I'll create a new #define, ELPP_HEADER_ONLY (which defaults to 1), and wrap the sections of the single header file that should be put in the source file within #if !ELPP_HEADER_ONLY/#endif. Then I'll write a script that extracts those sections of code to create the new header/source files.

This way all changes and pull requests are made against the single header file, but before making a new release you run the script to generate the split files.

@drescherjm
Copy link

I have wanted this to happen for a long time.

@abumq
Copy link
Owner

abumq commented Jan 3, 2017

Thanks aparajita, ELPP_HEADER_ONLY sounds very good with script to generate source file :)

@abumq
Copy link
Owner

abumq commented Jan 3, 2017

I have also added Release notes for next version, can you please rebase and add short description for this change under new heading NEW FEATURES

I will update README.md later (unless you are happy to do it)

@aparajita
Copy link
Contributor Author

@mkhan3189 Before I do this, I'd like to ask you to freeze the develop branch while I'm making the changes, which will take a few days. It's many hours of work, please don't make it more difficult by making me manually merge more changes in.

@abumq
Copy link
Owner

abumq commented Jan 3, 2017

I have created a branch elpp_single_header_changes based on current develop which is exclusively for your change. I will try as much to freeze the develop branch and should i require any change i will make sure you will not need to manually fix the conflicts.

But can you please make your changes in this new branch instead? (if you have already started making change just do git stash && git pull && git checkout elpp_single_header_changes && git stash apply (unless you know other secure ways than stashing) Hope this makes sense

@aparajita
Copy link
Contributor Author

Of course, basing it on a separate branch makes much more sense. Then it's your problem to merge with master. 😉

@abumq
Copy link
Owner

abumq commented Jan 3, 2017

Master branch don't get touched until release and yes it would be on me. Anything after things are merged to develop is on me :)

@aparajita
Copy link
Contributor Author

Let's discuss this a bit further. Here's what the release notes will essentially say:

NEW FEATURES

- You now have a choice: 1) you can add a single source file and a single #define to your project, or 2) you can save the 30 seconds it takes to add a single source file to your project, and forever more all of your compiles will take twice as long.

Given that choice, why would anyone actually choose 2?

The overall ease of using any library depends on three things:

  1. Ease of installation and configuration.
  2. Ease of using the API.
  3. Impact it has on the compile process.

The overall impact these have over the life of the project increases as we go down the list. #1 is a one-time occurrence, #2 affects every time the user writes code with your API, and #3 affects every compile.

Adding 30 seconds to #1 decreases the ease of installation, but over the life of the project it won't even be noticed. On the other hand, it will decrease #3 by 50% on every compile, which users will definitely notice — over and over again.

I'd say that's a huge win. And such being the case, why even offer the less attractive alternative?

@abumq
Copy link
Owner

abumq commented Jan 4, 2017

I agree with you on this. I think we can go with two files completely seperting declr with definitions.
This will also open gates to provide shared lib for linking if someone wants to use precompiled version.
I say we call it "easylogging++.cc"

Let me know where you're up to though

@aparajita
Copy link
Contributor Author

Great, I'll go ahead and do it.

I tried creating a static lib using the source file, and it compiled and linked fine, but there was no output. I didn't have time to figure out why. I'm sure we can get that to work eventually.

@aparajita
Copy link
Contributor Author

I'll work on it this weekend.

@abumq abumq added this to the v9.90 milestone Jan 8, 2017
@aparajita
Copy link
Contributor Author

First pass in #453.

@abumq
Copy link
Owner

abumq commented Jan 8, 2017

Opening this until it's stable and externs removed and samples/docs updated (to keep track)

@abumq abumq reopened this Jan 8, 2017
@easylogging
Copy link
Contributor

Pull request to merge to develop created. #455

Congrats :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants