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

[REG 2.067.0] Issue 14431 - huge slowdown of compilation speed #4784

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -100,8 +100,12 @@ void semanticTypeInfo(Scope *sc, Type *t)

// If the struct is in a non-root module, run semantic3 to get
// correct symbols for the member function.
// Note that, all instantiated symbols will run semantic3.
if (sd->inNonRoot())
if (TemplateInstance *ti = sd->isInstantiated())
{
if (ti->minst && !ti->minst->isRoot())
Module::addDeferredSemantic3(sd);
}
else if (sd->inNonRoot())
{
//printf("deferred sem3 for TypeInfo - sd = %s, inNonRoot = %d\n", sd->toChars(), sd->inNonRoot());
Module::addDeferredSemantic3(sd);
@@ -5968,22 +5968,35 @@ void TemplateInstance::semantic(Scope *sc, Expressions *fargs)
this->tnext = inst->tnext;
inst->tnext = this;

// If the first instantiation was in speculative context, but this is not:
if (tinst && !inst->tinst && !inst->minst)
if (minst && minst->isRoot() && !(inst->minst && inst->minst->isRoot()))

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 17, 2015

Member

What does that mean? If it has a name please abstract it to a function.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 17, 2015

Author Contributor

The first instantiation inst was not instantiated in root module (inst->minst == NULL means it was speculative, and inst->minst->isRoot() == false means it was in non-root module), but this is instantiated in root module.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 21, 2015

Member

A simple bool isSpeculative() { return minst == NULL; } would help.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 21, 2015

Member

So this selects the case where this is non-speculative and root, but the same instance inst was done in speculative or non-root context. Why would you want non-root instances here?

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 21, 2015

Author Contributor

If inst was done in speculative, it's already inserted in root module members. So the second time insertion is necessary only for non-root instances.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 21, 2015

Author Contributor

A simple bool isSpeculative() { return minst == NULL; } would help.

Unfortunately we already have Dsymbol::isSpeculative() and its meaning is not same with the condition tested in here. I'd really want to determine good name, but it's not discovered yet.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 22, 2015

Member

If inst was done in speculative, it's already inserted in root module members.

Why is that???

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 22, 2015

Author Contributor

If a template is instantiated in speculative context, the context should handle all errors during the instantiation - it's including the errors in function body.

void foo(T)(T t) { auto x = undefined_name; }
static assert(!is(typeof(foo(1))));   // the undefined error should be captured in here.

For that, all of speculative instantiations will be handled as root instances, and then, their semantic3 pass will surely get called.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

That's actually very simple and roughly means if (this->needsCodegen && !inst->needsCodegen).

This comment has been minimized.

Copy link
@9rnsr

9rnsr Aug 12, 2015

Author Contributor

Yes, that's right.

{
// Reconnect the chain if this instantiation is not in speculative context.
/* Swap the position of 'inst' and 'this' in the instantiation graph.
* Then, the primary instance `inst` will be changed to a root instance.
*
* Before:
* non-root -> A!() -> B!()[inst] -> C!()
* |
* root -> D!() -> B!()[this]
*
* After:
* non-root -> A!() -> B!()[this]
* |
* root -> D!() -> B!()[inst] -> C!()
*/
Module *mi = minst;
TemplateInstance *ti = tinst;
while (ti && ti != inst)
ti = ti->tinst;
if (ti != inst) // Bugzilla 13379: Prevent circular chain
inst->tinst = tinst;
}
minst = inst->minst;
tinst = inst->tinst;
inst->minst = mi;
inst->tinst = ti;

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 21, 2015

Member

Why swapping the contexts and appending inst instead of simply continuing with this which should already be member of a root module?

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 21, 2015

Member

It's a bad idea for debugging and understanding to change minst/tinst, just like you wouldn't want to change the scope of a symbol.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 21, 2015

Author Contributor

We need the second insertion at most only once. By the swap, inst is changed to a root instance, then the condition at line 5961 no longer hit. This is for the calculation amount reduction.

And, minst and tinst do not affect semantic analysis results. So the swap is not dangerous.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 22, 2015

Member

I still don't get it.
The whole template compilation/codegen pipeline is already complicated enough. We should avoid additional tricks, even more so if they are undocumented.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 22, 2015

Author Contributor

I updated to make the comment detailed more. Is that enough?

A poor solution is adding one bool flag bool TemplateInstance::isAlreadyInstantiatedInRoot; and test&check it here. But it will increment the size of TemplateInstance object. As far as possible I'd like to save memory footprint of dmd.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Jul 23, 2015

Member

As far as possible I'd like to save memory footprint of dmd.

You can get that for free by combining all the boolean flags to a FLAGS enum.
There is currently a hole for one bool anyhow ;).
https://github.com/D-Programming-Language/dmd/blob/61f8146e94dca8d3a0617d7235359b8e8b53e4b2/src/template.h#L312

This comment has been minimized.

Copy link
@9rnsr

9rnsr Jul 23, 2015

Author Contributor

I know the holes between bool variables, but it's unrelated refactoring, and should not go for the release.

Why you require such the redundant bool? Current "swap" is enough smart and efficient. I have no reason to degrade code quality.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

This is a really good argument to split TemplateInstantiation and TemplateInstance. This double usage had always confused me and likely eats up a lot of memory as well.
#4780 (comment)

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

So the point of this is that all identical TemplateInstances delegate to the first TemplateInstance (findExistingInstance) to avoid duplicate semantic. But if the first (primary) instance was done in speculative context, then no code will be generated. Therefore tinst and minst are swapped to make the primary instance a normal root instance.
It's still confusing that the scopes of this and inst won't match their tinst and minst variables.
It would be nice to move this into a shallowSwap function and to swap all fields that deal with the instantiation but not the instance, e.g. swap scope but not members.

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

Will the fact that a parent was a speculative template instance affect the semantic of members?
It should only change the error reporting, but maybe I'm overlooking something here.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Aug 12, 2015

Author Contributor

It's still confusing that the scopes of this and inst won't match their tinst and minst variables.

The scope of instantiated position, it's the Scope*sc parameter of TemplateInstance::semantic() function, is not saved anywhere. Therefore the mismatch of tinst/minst won't leak out of the function. And in TemplateInstance::semantic2() and semantic3(), the given Scope *sc parameter is just ignored.

Will the fact that a parent was a speculative template instance affect the semantic of members?

Never.


// If the first instantiation was speculative, but this is not:
if (!inst->minst)
{
// Mark it is a non-speculative instantiation.
inst->minst = minst;
if (minst) // if inst was not speculative
{
/* Add 'inst' once again to the root module members[], then the
* instance members will get codegen chances.
*/
inst->appendToModuleMember();
}
}

#if LOG
@@ -6005,66 +6018,10 @@ void TemplateInstance::semantic(Scope *sc, Expressions *fargs)

//getIdent();

// Add 'this' to the enclosing scope's members[] so the semantic routines
// will get called on the instance members. Store the place we added it to
// in target_symbol_list(_idx) so we can remove it later if we encounter
// an error.
#if 1
Dsymbols *target_symbol_list;
size_t target_symbol_list_idx = 0;
//if (sc->scopesym) printf("3: sc is %s %s\n", sc->scopesym->kind(), sc->scopesym->toChars());
if (0 && !tinst && sc->scopesym && sc->scopesym->members)
{
/* A module can have explicit template instance and its alias
* in module scope (e,g, `alias Base64Impl!('+', '/') Base64;`).
* When the module is just imported, normally compiler can assume that
* its instantiated code would be contained in the separately compiled
* obj/lib file (e.g. phobos.lib).
* Bugzilla 2644: However, if the template is instantiated in both
* modules of root and non-root, compiler should generate its objcode.
* Therefore, always conservatively insert this instance to the member of
* a root module, then calculate the necessity by TemplateInstance::needsCodegen().
*/
//if (sc->scopesym->isModule())
// printf("module level instance %s\n", toChars());

//printf("\t1: adding to %s %s\n", sc->scopesym->kind(), sc->scopesym->toChars());
target_symbol_list = sc->scopesym->members;
}
else
{
Dsymbol *s = enclosing ? enclosing : tempdecl->parent;
while (s && !s->isModule())
s = s->toParent2();
assert(s);
Module *m = (Module *)s;
if (!m->isRoot())
m = m->importedFrom;

//printf("\t2: adding to module %s instead of module %s\n", m->toChars(), sc->module->toChars());
target_symbol_list = m->members;

/* Defer semantic3 running in order to avoid mutual forward reference.
* See test/runnable/test10736.d
*/
if (m->semanticRun >= PASSsemantic3done)
Module::addDeferredSemantic3(this);
}
for (size_t i = 0; 1; i++)
{
if (i == target_symbol_list->dim)
{
target_symbol_list_idx = i;
target_symbol_list->push(this);
break;
}
if (this == (*target_symbol_list)[i]) // if already in Array
{
target_symbol_list = NULL;
break;
}
}
#endif
// Store the place we added it to in target_symbol_list(_idx) so we can
// remove it later if we encounter an error.
Dsymbols *target_symbol_list = appendToModuleMember();
size_t target_symbol_list_idx = target_symbol_list ? target_symbol_list->dim - 1 : 0;

// Copy the syntax trees from the TemplateDeclaration
members = Dsymbol::arraySyntaxCopy(tempdecl->members);
@@ -7335,6 +7292,83 @@ bool TemplateInstance::hasNestedArgs(Objects *args, bool isstatic)
return nested != 0;
}

/*****************************************
* Append 'this' to the enclosing module members[] so the semantic routines
* will get called on the instance members.
*/
Dsymbols *TemplateInstance::appendToModuleMember()
{
Module *md = tempdecl->getModule();

Module *mi = minst;
if (mi)
{
if (mi->isRoot())
{
/* If both modules mi and md where the template is declared are
* roots, instead use md to store the generated code in it.
*/
if (md && md->isRoot())
mi = md;

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

Again why this change? It seems unrelated to the fix.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Aug 12, 2015

Author Contributor

This is not necessary to fix the regression, but it will increase stability for the position where the instance will be inserted. For example:

a.d

import ...;

template X() { ... }

X!()

b.d

import a, ...;

X!()

With the command line dmd -c a.d b.d:

  • If this adjustment does not exist, the instance X!() will be inserted to the member of either a or b. The actual position will depend on the semantic order.
  • If this adjustment exists, the instance X!() will be always inserted to the member of a.

By that, the X!() code will be always stored into a.obj, in that case. I think the stable algorithm result is better than unstable, if there's not so much performance difference.

}
else
{
/* A module can have explicit template instance and its alias
* in module scope (e,g, `alias Base64Impl!('+', '/') Base64;`).
* When the module is just imported, normally compiler can assume that
* its instantiated code would be contained in the separately compiled
* obj/lib file (e.g. phobos.lib).
*/
/*
* Bugzilla 2644: However, if the template is instantiated in both
* modules of root and non-root, compiler should generate its objcode.
* Therefore, always conservatively insert this instance to the member of
* a root module, then calculate the necessity by TemplateInstance::needsCodegen().
*/
}
//printf("\t1: adding to %s\n", mi->toPrettyChars());
}
else
{
// 'this' is speculative instance
if (md && md->isRoot())
{
mi = md;
}
else
{
// select arbitrary root module
Dsymbol *s = enclosing ? enclosing : tempdecl->parent;
while (s && !s->isModule())
s = s->toParent2();
assert(s);
mi = (Module *)s;
if (!mi->isRoot())
mi = mi->importedFrom;
}
assert(mi->isRoot());
//printf("\t2: adding to module %s instead of module %s\n", mi->toPrettyChars(), sc->module->toPrettyChars());
}

Dsymbols *a = mi->members;
for (size_t i = 0; 1; i++)
{
if (i == a->dim)
{
a->push(this);
if (mi->semanticRun >= PASSsemantic3done && mi->isRoot())
Module::addDeferredSemantic3(this);
break;
}
if (this == (*a)[i]) // if already in Array

This comment has been minimized.

Copy link
@MartinNowak

MartinNowak Aug 12, 2015

Member

Why would it already be in the array?
Seems like noone should call appendToModuleMember multiple times or for TIs that already are root module members.
Might be better to make this an assert to not hide bugs elsewhere.

This comment has been minimized.

Copy link
@9rnsr

9rnsr Aug 12, 2015

Author Contributor

I also think the conflict would not happen, but I'm really not sure it yet, because same check existed in old code. So I'd just keep it until we can sure it is unnecessary.

{
a = NULL;
break;
}
}
return a;
}

/****************************************
* This instance needs an identifier for name mangling purposes.
* Create one by taking the template declaration name and adding
@@ -351,6 +351,7 @@ class TemplateInstance : public ScopeDsymbol
bool findBestMatch(Scope *sc, Expressions *fargs);
bool needsTypeInference(Scope *sc, int flag = 0);
bool hasNestedArgs(Objects *tiargs, bool isstatic);
Dsymbols *appendToModuleMember();
void declareParameters(Scope *sc);
Identifier *genIdent(Objects *args);
void expandMembers(Scope *sc);
@@ -13,8 +13,11 @@ struct S
to!string(false);
// [1] to!string(bool src) should be deduced to pure @safe, and the function will be mangled to:
// --> _D8std141984conv11__T2toTAyaZ9__T2toTbZ2toFNaNbNiNfbZAya
// [2] its object code should be stored in the library file, because it's instantiated in std14188.uni:
// [2] its object code would be stored in the library file, because it's instantiated in std14188.uni:
// --> FormatSpec!char --> to!string(bool src) in FormatSpec!char.toString()
// But semanti3 of FormatSpec!char.toString() won't get called from this module compilation,
// so the instantiaion is invisible.
// Then, the object code is also stored in test14198.obj, and the link will succeed.
}
else
static assert(0);
@@ -0,0 +1,21 @@
module imports.test14901a;

//extern(C) int printf(const char*, ...);

extern extern(C) __gshared static int initCount;

int make(string s)()
{
__gshared static int value;

struct WithCtor
{
shared static this()
{
//printf("%s\n", s.ptr);
initCount++;
}
}

return value;
}
@@ -0,0 +1,13 @@
module imports.test14901b;

import imports.test14901a;

alias bar = make!"bar";

struct User(int id)
{
int foo()
{
return bar;
}
}
@@ -0,0 +1,10 @@
module imports.test14901c;

import imports.test14901b;

shared static this() {}

void caller1()
{
User!1 u;
}
@@ -0,0 +1,8 @@
module imports.test14901d;

import imports.test14901b;

void caller2()
{
User!2 u;
}
@@ -13,13 +13,13 @@ else
fi
libname=${dir}${SEP}lib14198b${LIBEXT}

# Do link failure without library file.
# Do not link failure even without library file.

$DMD -m${MODEL} -I${src} -of${dir}${SEP}test14198b${EXE} ${src}${SEP}test14198.d > ${output_file} 2>&1
grep -q "_D8std141984conv11__T2toTAyaZ9__T2toTbZ2toFNaNbNiNfbZAya" ${output_file} || exit 1
grep -q "_D8std141984conv11__T2toTAyaZ9__T2toTbZ2toFNaNbNiNfbZAya" ${output_file} && exit 1

$DMD -m${MODEL} -I${src} -of${dir}${SEP}test14198b${EXE} -version=bug14198 ${src}${SEP}test14198.d > ${output_file} 2>&1
grep -q "_D8std141984conv11__T2toTAyaZ9__T2toTbZ2toFNaNbNiNfbZAya" ${output_file} || exit 1
grep -q "_D8std141984conv11__T2toTAyaZ9__T2toTbZ2toFNaNbNiNfbZAya" ${output_file} && exit 1

rm ${dir}/{test14198b${OBJ},test14198b${EXE}}

@@ -0,0 +1,20 @@
// REQUIRED_ARGS:
// PERMUTE_ARGS: -unittest
// EXTRA_SOURCES: imports/test14901a.d imports/test14901b.d imports/test14901c.d imports/test14901d.d
// COMPILE_SEPARATELY

module test14901;

import imports.test14901c;
import imports.test14901d;

extern(C) __gshared static int initCount;

extern(C) int printf(const char*, ...);

void main()
{
caller1();
caller2();
assert(initCount == 1);
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.