fix Issue 14508 - [REG2.067.0] compiling with -unittest instantiates …

[WalterBright]

…templates in non-root modules

https://issues.dlang.org/show_bug.cgi?id=14508

This is an alternative to Kenji's fix #4626 which is a fix for a regression introduced by #4384. It essentially reverts part of 4384.

The problems with 4384:

1. it introduces, in some cases, a 30% slowdown in compile speed due to an excessive number of templates being instantiated.
2. sometimes it instantiates too many templates, resulting in another very complex fix, 4626. I do not understand 4626. I fear the logic in template instantiations is already far too complex, and this takes it to another level.

So, 4626 was introduced to solve a problem https://issues.dlang.org/show_bug.cgi?id=2644 where a template wasn't being instantiated. We already have a solution to this, the -allinst compiler switch. I admit it's a crude approach, but it works, and is fairly easy to understand, explain to others, and implement. Yes, that switch slows down compilation too, because it instantiates everything in sight, but the cost is only borne by those who insist on creating complex circular template imports, not everyone else.

4626 also has a fix for https://issues.dlang.org/show_bug.cgi?id=10920 which I leave intact.

[WalterBright]

Hmm, now I'm getting all sorts of weird failures in the test suite for things that appear unrelated. I fear I have no idea how all this works after all these revisions.

[9rnsr]

I dislike this change. -allinst is a hack and should be removed in the future. Relying on that is merely wrong.

[WalterBright]

@9rnsr I understand, but it works and is straightforward. The other proposed solutions come with problems I mentioned that are worse.

[9rnsr]

I found that this PR will introduce another issue.

After this change, issue 2500 case will become unsupported without -allinst. But, the issue case would happen when someone tries to use separate compilation style in his project building. I think it's not uncommon. Then, in practical, separate compilation style building without -allinst will be impossible, because of the random link-failure.

To fix that, users need to add -allinst for each compilations, but it would introduce huge slowdown of compilation speed.

In short, this change will request huge performance regression in practical projects. I cannot accept this.

[WalterBright]

I suggest pulling this one for now, especially for the 2.068 release, as it puts us back into a known state. The other suggested PRs are complex and risky for adding to a beta that is being prepared for release.

[9rnsr]

Yes, this PR will change back things to the known state, along with the huge side effect. I believe it will be called "regression".

On the other hand, my PR has no known side effect, although it's a little complex than this.

From my experience, a bugfix is always risky (let's consider the number of regressions wchich introduced by the previous bug fixes). Therefore I think it would not be so much reason in this case.

Sorry, at least I cannot accept this change.

[MartinNowak]

It think it's the right place to fix the issue (see
#4384 (comment) or https://issues.dlang.org/show_bug.cgi?id=14431#c12), but we can't simply disable template generation in non-root modules, while not instantiating templates that are already in non-root modules.
This would set us back to the horrible state after adding the instantiation optimization, meaning many people will have linking issues. It's even worse nowadays b/c of dub and packages.
The -allInst is not a solution b/c it makes compilation much much slower.
We'd also need to explain everyone when, why, and how to use it (with dub), meaning we spent the next 4 weeks with support for linkage issues.

[MartinNowak]

I'd suggest that we do not fix the template and compile time issue for 2.068.0. Clearly this pull will break many projects, and -allinst is not an appropriate workaround. Also you can't seem to come to a conclusion with Kenji about #4784.

[MartinNowak]

Outline of a better template instantiation approach.

• We split TemplateInstance into 2 classes, TemplateInstantiation for plain parsing, and TemplateInstance for the semantic part
• During semantic we resolve all TemplateInstantiations to a single TemplateInstance using the mangling prefix of the template (TemplateDeclaration + TiArgs) and a hash table
• TemplateInstance keeps track of all scopes/modules where it was instantiated
• TemplateInstantiations forward semantic3 to their TemplateInstance
• After semantic3 we pass over all TemplateInstances and compute the module that owns the TemplateInstance from the instantiation contexts. This must resolve the mutual import problem.
• Then we distribute all the TemplateInstances to their owning module for codegen (or not if owned by a non-root module)

This is very similar to the current scheme, but getting rid of the ti->tnext chain and the module->members->append complications will already simplify this a lot.
If we then can find a good way to compute stable owners for TemplateInstance while reusing as many TemplateInstances from non-root modules as possible, we might have a sustainable solution.

[WalterBright]

This should have gone into 2.068. We know that it works. We got ourselves into a jolly mess with the other schemes.

[MartinNowak]

This should have gone into 2.068. We know that it works. We got ourselves into a jolly mess with the other schemes.

We know that it doesn't work Walter. That's the state you introduced with the non-root module optimization. It broke about every other project and Kenji had to clean up all the mess.
Also -allinst is not a solution, it's a workaround for a buggy implementation. Suddenly requiring it will also break many projects.

[WalterBright]

That's the state you introduced with the non-root module optimization. It broke about every other project and Kenji had to clean up all the mess. Also -allinst is not a solution, it's a workaround for a buggy implementation. Suddenly requiring it will also break many projects.

It's not suddenly requiring it. It was required back in 2.066. There was no disaster with it - a few projects added -allinst and it was good. Then all the complexity came in, with compiler slowdowns, and repeated regressions.

[WalterBright]

After this change, issue 2500 case will become unsupported without -allinst

2500 is when modules mutually import each other. That indeed is a problem with this PR, but can be fixed by checking for mutually importing modules. It's not necessary to come up with the overcomplex other schemes.

[9rnsr]

can be fixed by checking for mutually importing modules.

I'm not sure what you are saying. The line 7937 you're commenting out in template.c is just the check code for the mutual importing. You've exactly tried to kill the fix for issue 2500. Didn't you recognize what you're doing?

In there: If minst->isRoot() is false (minst is non-root module) but minst->rootimports() is true (minst imports any root modules), it means a mutual importing.
Of course, all of non-root modules are directly or indirectly imported by one or more root modules.