Reduce template function instantiations related to array equality

[n8sh]

This is achieved by splitting off the all-scalar case and by moving the at and trustedCast helper functions out of the body of core.internal.array.equality.__equals.

[dlang-bot]

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21030	enhancement	Reduce template function instantiations related to array equality

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 + druntime#3152"

[n8sh]

Pushing to fix a comment x 2

[UplinkCoder]

not sure you need the length == 0 case.

[UplinkCoder]

it's an optimisation, for an uncommon case, therefore a slowdown for the common case.

[n8sh]

It's there because I am under the impression that memcmp has undefined behavior for null pointers. If not it can be removed.

[UplinkCoder]

@n8sh you are correct.
memcpy on null is undefined

[UplinkCoder]

@n8sh Have you verified that this helps?

[n8sh]

@UplinkCoder Yes, I wrote a long program that was nothing but array equality checks, and with the change compiling it takes about 23% as much time and about 22% as much memory.

[n8sh]

Copied from #3142

[kinke]

I've rebased and updated #3142 in the meantime too. I don't really want to rebase yet another time after pieces of it are ripped out into yet another separate PR (and each with its own dlang issue? Is that tracker spamming (ab)used for changelog generation?).

[n8sh]

(and each with its own dlang issue? Is that tracker spamming (ab)used for changelog generation?)

I am under the impression that any non-trivial PR should have an associated issue.

[n8sh]

Anyway, if this PR is merged before that one (which seems possible because this PR isn't waiting on anyone else's work), I'd be fine with rebasing #3142 for you. Regardless of which PR is pulled first the other will need to be rebased anyway.

[kinke]

It'd be nice to have a comparison with your test program, but the non-identical semantics (also wrt. memcmp application) don't allow a direct comparison. Anyway, getting rid of superfluous instantiations was obviously my main goal after seeing the original code, and the .tupleof thing an unexpected hold-up. [Edit: And AFAICT, this PR shouldn't be needed after mine.]

[n8sh]

If I remove all of this PR except the addition of the scalar-specific __equals definition and the addition of a template constraint, that would minimize conflict while still doing something useful performance-wise.

[kinke]

For the time being, maybe, but my version, AFAICT, doesn't need any of this, so I'd basically just revert this and apply mine instead. In my version, there's no trustedCast() helper template at all anymore. at was extracted to the module-level, just like you did (but does the void->ubyte promotion in my case, instead of trustedCast and the void[] special cases for __equals). I don't use a 2nd __equals, but a little nested trustedMemcmp helper (in a static if for memcmp-able arrays only).

[n8sh]

It'd be nice to have a comparison with your test program, but the non-identical semantics (also wrt. memcmp application) don't allow a direct comparison.

With those provisos, yours compiles in 75% as much time and using 60% as much memory as the baseline.

[n8sh]

I don't use a 2nd __equals

Right, but doing so improves compilation speed without interfering with any of the stuff you are doing.

[kinke]

Your version doesn't use memcmp for static array element types etc. though, that's why runtime performance would be interesting as well. Please post your test code as gist or so somewhere, so that I can experiment as well.

[n8sh]

Your version doesn't use memcmp for static array element types etc. though

Right, and static arrays are handled by the non-scalar version (since __isScalar is never true for static arrays) with your memcmp check etc.

Please post your test code as gist or so somewhere, so that I can experiment as well.

Sure. You'll see that the code does nothing but test whether there is a net benefit in compilation time for the happy path given the additional work done in template resolution. My starting assumption was that array equality checks are primarily performed between arrays of scalars so that is the case I was focused on improving. https://pastebin.com/d1A5f07s

[kinke]

Thx a lot, I'll look into it another time - I hope(d) to have covered your happy path optimization in a not-unnecessarily-complex useMemcmp check; if it doesn't, that's definitely an improvement of yours.