gflow.d: double speed of fillInDNunambig()

[WalterBright]

@ibuclaw notes its quadratic performance here https://issues.dlang.org/show_bug.cgi?id=6401#c4

This change doubles its speed by noting that the matrix it creates is symmetric, so no need to redo half the work. But it's still quadratic.

The change:

1. splits off allocation of the DNunambig vectors from their computation
2. the inner loop starts at start rather than 0
3. removes extra checks as being redundant with the DNunambig vector being non-null

It isn't obvious that it is correct. So I added a check that computed the vectors the old way, and compared for identity with the new results. After it passes, this check can be deleted as being too expensive.

[dlang-bot]

Thanks for your pull request, @WalterBright!

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 run digger -- build "master + dmd#12459"

[WalterBright]

Hmm, it passes on my local machine.

[WalterBright]

Looks like adding that check was a good idea, it found two bugs in the new version.

[RazvanN7]

splits off allocation of the DNunambig vectors from their computation

Why is this necessary? How does this help performance?

After it passes, this check can be deleted as being too expensive.

It's green now.

[ibuclaw]

splits off allocation of the DNunambig vectors from their computation

Why is this necessary? How does this help performance?

Originally the nested foreach loop of 0..n meant number of iterations it did was (n*n), now the number of iterations would be around (n*n/2) (edit: actually to be precise (n*n/2)+n/2) - which as Walter notes is still quadratic.

[ibuclaw]

I guess this can be disabled now.

[WalterBright]

removed

[WalterWaldron]

I've posted a rough draft of a more linear solution here:
master...WalterWaldron:dnumambig

Once the hash helper functions are moved to an appropriate location, the resulting body of fillInDNunambig is quite concise.

It doesn't solve extreme degenerate cases such as https://issues.dlang.org/show_bug.cgi?id=7157 but it should out do this solution.

[ibuclaw]

Bench 1 (Re-running because I'm getting different results now - I guess the background noise on my machine must be busier than usual)

Baseline bfad3f0	#12409	#12453	#12455	#12456	#12459	@WalterWaldron 0ea9d77	Baseline + WalterWaldron	vs. GDC
32.543s	38.480s	36.161s	35.940s	36.314s	33.694s	32.649s	27.361s	0.985s

Bench 2

Baseline bfad3f0	#12409	#12453	#12455	#12456	#12459	@WalterWaldron 0ea9d77	Baseline + WalterWaldron	vs. GDC
1m23.593s	1m34.041s	1m32.060s	1m32.213s	1m32.993s	1m24.946s	1m22.961s	1m17.279s	7.645s

At best, it's a marginal gain. Should really profile it to drill down a bit more on the lowest hanging fruit. As fixing this function isn't giving a satisfying boost, if the goal is to be as fast as gdc.

[WalterWaldron]

At best, it's a marginal gain. Should really profile it to drill down a bit more on the lowest hanging fruit. As fixing this function isn't giving a satisfying boost, if the goal is to be as fast as gdc.

True, it is simply the easiest of the problem functions to improve (i.e. a first step,) I'm planning to tackle the other O(N^2) backend functions mentioned in https://issues.dlang.org/show_bug.cgi?id=6401#c4 :

• gother.d:accumda is simple to fix
• The newly posted mscoffobj.d:MsCoffObj_getsegment is also a simple fix.
• gflow.d:accumaecpx and gdag.d:aewalk are more complicated

It all boils down to using hash tables instead of O(N) search for each of the problem functions.

[RazvanN7]

ping @WalterBright

[RazvanN7]

@WalterBright what are your plans with this PR?

[RazvanN7]

ping @WalterBright

[WalterBright]

I recommend pulling this and then when @WalterWaldron 's solution is ready, pull that one.

[RazvanN7]

Thanks!