Skip to content

Comments

[WIP] Rework -unittest flag #10759

Closed
alexandrumc wants to merge 6 commits intodlang:masterfrom
alexandrumc:rework-unittest
Closed

[WIP] Rework -unittest flag #10759
alexandrumc wants to merge 6 commits intodlang:masterfrom
alexandrumc:rework-unittest

Conversation

@alexandrumc
Copy link
Contributor

@alexandrumc alexandrumc commented Feb 3, 2020

The idea of this PR is to decrease the compilation time when the -unittest switch is used.

This has to do with the way the current algorithm that generates code for template instantiations works. The algorithm has two branches:

  • If -unittest is used, then do code generation for every template that is instantiated and used in a root module, even if the same template is instantiated in some non-root module that is imported.

  • Else (unittests not enabled) prefer instantiations from non-root modules, to minimize object code size. If a template is ever instantiated by non-root modules, do not generate code for it, because it will be generated when the non-root module is compiled.

So there are 3 options in my opinion:

  1. the first one is to eliminate the branching and to adopt in every case the same behaviour performed when the -unittest switch is used: this might decrease the compile-time and will generate huge binaries because for almost every template instantiation code will be generated, but there will be a unified behaviour in the compiler

  2. make the non-unittest behaviour the default, keep binaries small, decrease compilation time when -unittest is enabled but break a lot of code and force people to change the way they compile and write their code

  3. make a better code generation algorithm for templates (that will use some kind of analysis to determine on which template instantiations code generation should be called and on which shouldn’t be called)
    1. this probably will lead to an increase in the binary size (not a huge one, but most likely we won’t be able to do something fine-grained because of the amount of work/time needed)
    2. and probably will break some code but not that much and will also decrease the -unittest compilation time

So far I've eliminated the branching from the algorithm and I've made the "if -unittest is used" behaviour the default one. The next step is to do some measurements regarding the compilation time and based on them to decide what should be done further. I'll update this description with the measurements when I'll have them.

@alexandrumc alexandrumc requested a review from Geod24 as a code owner February 3, 2020 16:02
@dlang-bot dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Feb 3, 2020
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @alexandrumc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10759"

@MoonlightSentinel
Copy link
Contributor

It should be possible to revert to the old behavior. Maybe extend the -unittest flag to take an optional value, e.g. -unittest=all and -unittest=rootonly

@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 21, 2020

@alexandrumc Please add an explanation for all of these changes so that we don't have to speculate what the intent is. I assume that you haven't done so because this is a work in progress project, but I suggest that you do it anyway so that reviewers/community members are able to understand what you are trying to do and offer support.

@alexandrumc
Copy link
Contributor Author

@alexandrumc Please add an explanation for all of these changes so that we don't have to speculate what the intent is. I assume that you haven't done so because this is a work in progress project, but I suggest that you do it anyway so that reviewers/community members are able to understand what you are trying to do and offer support.

Done.

@Geod24
Copy link
Member

Geod24 commented Feb 24, 2020

You might also want to test what effect your change have on some sample code, e.g. the ones at https://perf.dlang.io/

@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 25, 2020

How about creating a file __tempinstances.d that will be viewed as a root module where all the template instances from non-root modules go? This file is generated by the compiler and it imports the non-root modules that were the source of a template instantiation. All the root modules that instantiate templates from non-root modules will import __tempinstances. This way, every time a template is instantiated from a non-root module, it is going to have code generated for it in an auxiliary file that is going to be linked by default.

Destroy.

@atilaneves
Copy link
Contributor

How about creating a file __tempinstances.d that will be viewed as a root module where all the template instances from non-root modules go? This file is generated by the compiler and it imports the non-root modules that were the source of a template instantiation. All the root modules that instantiate templates from non-root modules will import __tempinstances. This way, every time a template is instantiated from a non-root module, it is going to have code generated for it in an auxiliary file that is going to be linked by default.

Destroy.

Importing __tempinstances might cause name clashes. I'm also unsure of what this strategy would accomplish.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 26, 2020

Importing __tempinstances might cause name clashes.

Why would you get name clashes?

I'm also unsure of what this strategy would accomplish.

You will generate code for each template instance exactly once no matter in what module the instantiation is used + you get uniformity for the compilation with -unittest and without it. Plus the temp file can be cached. For example, if you have 2 files a.d and b.d that both use the same template instance from the non-root module c.d and you want to compile separately, first a and then b, once you compile b the template instance from a is already there in __tempinstances.

@RazvanN7
Copy link
Contributor

This seems to be obsolete now, given the recent template emission fixes. @alexandrumc please reopen if anything new is added.

@RazvanN7 RazvanN7 closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Needs Rebase Review:stalled Review:WIP Work In Progress - not ready for review or pulling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants