Merge remote-tracking branch 'upstream/mangle' into merge_mangle

[MartinNowak]

No description provided.

[dlang-bot]

Thanks for your pull request, @MartinNowak!

Bugzilla references

Auto-close	Bugzilla	Description
✓	15831	IFTI voldemort type exploding bloat

[MartinNowak]

Thanks Rainer for your persistent work on this ;).

[MartinNowak]

Looks like vibe-d found a small phobos problem with this.
https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fdmd/detail/PR-6998/1/pipeline#step-526-log-30

/var/lib/jenkins/dlang_projects/distribution/bin/../imports/std/traits.d(1063,21): Error: slice [38..32] exceeds array bounds [0..32]

/var/lib/jenkins/dlang_projects/distribution/bin/../imports/std/traits.d(1063,21): Error: template instance std.traits.ParameterStorageClassTuple!(void function(HTTPServerRequest, HTTPServerResponse)).demangleNextParameter!("CQBlQBjQBh18HTTPServerResponseZv", 1LU) error instantiating

[rainers]

fail12485.d is a rather ugly test: it wants to produce an error on recursion, but does so only by the length of the symbol. Should we add a recursion check elsewhere?

[rainers]

druntime failures are due to mangled names being compared by profiler tests. Can we assume ddemangle to be available to compare demangled names?

[MartinNowak]

druntime failures are due to mangled names being compared by profiler tests. Can we assume ddemangle to be available to compare demangled names?

I'd rather not as it adds a dependency on an up-to date ddemangle being installed on all CI systems.

[rainers]

I'd rather not as it adds a dependency on an up-to date ddemangle being installed on all CI systems.

I've opted to allow both manglings for now, the old version can be removed later: dlang/druntime#1878

[rainers]

Now the druntime patch is merged, I see two issues left:

• jenkins fails because reimplement std.traits.ParameterStorageClassTuple() phobos#5427 has been reverted. IIUC __traits(getParameterStorageClassTuple) also caused errors in vibe.d. Is anyone working on this? @WalterBright

• fail12845.d now doesn't stop on a mangled symbol component being longer than 8MB, but if the limit of 300 recursive template instantiations is hit. This needs about 5 GB of memory, though. Some machines don't seem to have that much available, i.e. work-vm1, so the build bails out with out-of-memory or is killed otherwise. With 32-bit builds of dmd on Windows, the out-of-memory error code just happens to be 1, as expected by d_do_test for a normal failure.
  Should the test actually try to generate a symbol longer than 8MB or try to hit that recursion limit with less memory?

[rainers]

Should the test actually try to generate a symbol longer than 8MB or try to hit that recursion limit with less memory?

@MartinNowak I opted for the former here: MartinNowak#2. Can you merge this into this PR? Or can I do this myself? I'm not used to the workflow, but is this PR open for edits from other?

[wilzbach]

but is this PR open for edits from other?

Yes.

[wilzbach]

I'm not used to the workflow, but is this PR open for edits from other?

You can also close this PR and open one from your branch if that's easier for you.

[rainers]

but is this PR open for edits from other?

Yes.

AFAICT it's not visible in the side bar, only for my own PRs.

You can also close this PR and open one from your branch if that's easier for you.

Thanks for the info. I just pushed to Martins fork, seems to work fine. It even automatically closed my PR against his fork.

[quickfur]

Just wondering if this will get merged soon. Getting impatient waiting for this to land in master. ;-)

[wilzbach]

AFAICT it's not visible in the side bar, only for my own PRs.

FYI: it's available when you see the following before the CI status window:

Add more commits by pushing to the merge_mangle branch on MartinNowak/dmd.

(Currently on my phone, so without screenshot)

If you do this more frequently, you might want to have a look at this page:

https://wiki.dlang.org/Guidelines_for_maintainers#Write_access_to_PRs

[rainers]

@braddr I'm afraid that a bad test in a previous commit continues to allocate memory, but never stops on some build servers even though the build is deleted. One seems to build for 55 hours now: https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=2670646&isPull=true

Can you abort these somehow?

[rainers]

FYI: it's available when you see the following before the CI status window:

Thanks for the info, found it now. I've seen it before but thought that it would be always displayed.

[wilzbach]

Now the druntime patch is merged, I see two issues left:

I think the Phobos PR has been merged and the auto-tester is all green. Any other blocking issues? :)

[rainers]

Any other blocking issues? :)

No objections from my side ;-)

BTW:

• there is also a corresponding dlang.org PR: document changes to mangling of type and identifier back refs dlang.org#1664
• most build servers seem to have recovered, but AFAICT one tester still builds this PR: https://auto-tester.puremagic.com/hosts/details.ghtml?hostid=37 and does nothing else. Can someone stop that job?

[wilzbach]

No objections from my side ;-)

Noice! 🎉
I retested this branch and now the performance problems are gone. On the contrary:

Library	master	mangle
Druntime	6.5s	6.3s
Phobos unittest-debug	2:20 min	2:16min

I also tried building Vibe.d, but there's a huge variance of the run time when running the same build multiple times (of course with --force).

most build servers seem to have recovered, but AFAICT one tester still builds this PR: https://auto-tester.puremagic.com/hosts/details.ghtml?hostid=37 and does nothing else. Can someone stop that job?

Yeah ... only @braddr has access to the auto-tester

[wilzbach]

Root package dpl-docs references unknown package ddox

FYI: this just means that code.dlang.org was unreachable. There are multiple efforts to make DUB more stable, e.g. dlang/dub#1190, dlang/dub#1198, dlang/dub-registry#231

[rainers]

Added a changelog entry.

The vibe.d failure looks like a tough one to fix: it uses the mangled name during serialization.

Not sure how not to break this, but adding a function that converts the new mangling to the old one could help updating code...

[rainers]

Rebased and fixed the mangling of the new builtins.

Github provides refs/pull/*/merge remote refs that are already merged with the target branch.

Thanks for the info, I missed the additional "merge" in the ref.

[MartinNowak]

We're now testing vibe.d 0.8.1-rc.2 with the mangling dependency gone.
Should we still merge this story into stable for 2.076.0 (2017-09-01) @rainers? Beta began 2 days ago.
FWIW all the necessary druntime changes are in, right?

[wilzbach]

Should we still merge this story into stable for 2.076.0 (2017-09-01)

AFAIK was vibe.d release the only thing blocking this from being merged.
@MartinNowak: I think it's your call. IIRC you want to keep the release schedule more firmly and this story could potentially disrupt it, but there's also a huge gain from it.

In any case, let's finally merge this into master :)

[quickfur]

Yes, let's merge this into master! Can't wait...

[rainers]

Should we still merge this story into stable for 2.076.0 (2017-09-01) @rainers?

Two weeks is probably too short for proper exposure, so master should be fine. I guess those interested in this most have no trouble building it themselves or using dmd nightly.

[quickfur]

I've been checking out the branch separately and rebasing on master locally. Works beautifully, if only a bit inconveniently because every time I update git master I have to manually rebase. It will be much easier once this is merged into master. Also, it will allow more people to give it the testing it needs before we ship it with a release, on the off-chance that some remote corner-case causes a regression.

[quickfur]

WOOOHOOOO!! Finally, this is in!

Big thanks to everyone involved in the effort in fixing this issue. Special kudos to @rainers for a totally awesome implementation.

[rainers]

Let's hope it lives up to the expectations.

Thanks everyone helping to get this through the different CI hosts and finally merged (after more than a year). This raises hope for the precise GC, too (the original PR is open for almost 3 years, but rebooted last year dlang/druntime#1603).

[quickfur]

I've been using the mangle branch before it was merged to master for my template-heavy project for the past 2 weeks or so. Rebased it multiple times on git HEAD as new PRs were merged into master. So far, I have not seen any problems, and it has helped keep my executable sizes sane (~4MB for 2 executables, instead of 63MB and 186MB(!) respectively, without the mangle branch).

[quickfur]

Hopefully other template-heavy projects will see similar improvements too.

[quickfur]

Tested the latest dmd on some of my other D projects. Overall trend is a decrease in executable size. The size reductions are generally less dramatic for non-template-heavy code, but for template-heavy code, dramatic gains similar to what I mentioned are noted. For example, dcal saw a reduction from about 7-8 MB to 1.9MB, which is roughly a 3x-4x reduction. The amount of reduction roughly correlates with how template-heavy the code is, as is to be expected.

I did not observe any increases in executable sizes except in a few very old projects that I'm guessing are not valid data points, because I may have run strip and/or upx on them the last time I built them.

[ibuclaw]

I've lodged the change to fail12485.d as a regression https://issues.dlang.org/show_bug.cgi?id=21499