Fix Issue 8156 - Optimize string comdats name generation (Windows)

[BorisCarvajal]

Replace md5 checksum with the already calculated hash in stringTab, and don't call mangleToBuffer in advance if we know the string is long enough .

Test case:

void main()
{
   auto i = import("bigfile"); // tested with 102mb file
}

Tested using -c -J. test.d --DRT-gcopt=profile:1 -lowmem on QEMU.

Old behavior:

-m32 switch:
Time: 3m07s, RAM: 779mb <-- really slow, mostly handled by #11554
-m32mscoff switch:
Time: 4.7s, RAM: 779mb
-m64 switch:
Time: 4.7s, RAM: 779mb

New behavior (DMD built with #11554 patch too):

-m32 switch:
Time: 2.1s, RAM: 319mb
-m32mscoff switch:
Time: 2s, RAM: 319mb
-m64 switch:
Time: 2s, RAM: 319mb

[dlang-bot]

Thanks for your pull request and interest in making D better, @BorisCarvajal! 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

Auto-close	Bugzilla	Severity	Description
✓	8156	critical	Very slow compilation with string-imported file ~100 MiB

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

Testing this PR locally

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

dub run digger -- build "master + dmd#11555"

[ibuclaw]

Old behavior:

-m32 switch:
Time: 3m07s, RAM: 779mb <-- really slow, mostly handled by #11554
-m32mscoff switch:
Time: 4.7s, RAM: 779mb
-m64 switch:
Time: 4.7s, RAM: 779mb

New behavior (DMD built with #11554 patch too):

-m32 switch:
Time: 0.7s, RAM: 319mb
-m32mscoff switch:
Time: 0.6s, RAM: 319mb
-m64 switch:
Time: 0.6s, RAM: 319mb

I think your benchmark should have multiple modules that import bigfile. Compile separately and link together. There should be no observed executable size regressions.

[BorisCarvajal]

Old behavior:
-m32 switch:
Time: 3m07s, RAM: 779mb <-- really slow, mostly handled by #11554
-m32mscoff switch:
Time: 4.7s, RAM: 779mb
-m64 switch:
Time: 4.7s, RAM: 779mb
New behavior (DMD built with #11554 patch too):
-m32 switch:
Time: 0.7s, RAM: 319mb
-m32mscoff switch:
Time: 0.6s, RAM: 319mb
-m64 switch:
Time: 0.6s, RAM: 319mb

I think your benchmark should have multiple modules that import bigfile. Compile separately and link together. There should be no observed executable size regressions.

I just tested with the corrections and there's no size regressions.

[WalterBright]

I have serious reservations about this. The hash in stringTab is optimized for speed, not uniqueness. Any collisions are handled by a linear search.

The purpose of the md5 hash is to ensure uniqueness - not just within the object file, but across all the object files linked together (so identical strings share storage).

The hash has to be unique. Using stringTab's hash does not meet this requirement.

I recommend instead looking at speeding up the md5 calculation. Here is some work done on speeding up sha:

dlang/phobos#7534

[BorisCarvajal]

@WalterBright That concern is valid, so I've discarded the md5 replacement, still the other changes(*) bring like 55% speed up (DMD not in release mode though).

(changes):

• Don't call mangleToBuffer in advance if we know the string is long enough.
• Checksum over the original string not the mangled one.

[BorisCarvajal]

@MoonlightSentinel, Why the "needs tests" tag? Everything should already be covered by the test suite.

[BorisCarvajal]

Can this be merged?