Page MenuHomeSolus

Add LTO for argon2
ClosedPublic

Authored by livingsilver94 on Mon, Oct 28, 5:09 PM.

Details

Summary

Add LTO for argon2. LTO on my system gives an average reduction in time by 3.63260%. Tests were performed with the builtin benchmark and the gain in speed calculated with a script of mine.

Test Plan

KeepassXC still opens my password database.

Diff Detail

Repository
R4080 argon2
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

livingsilver94 created this revision.Mon, Oct 28, 5:09 PM
livingsilver94 requested review of this revision.Mon, Oct 28, 5:09 PM
livingsilver94 updated this revision to Diff 18024.

Actually enable LTO

livingsilver94 edited the summary of this revision. (Show Details)Mon, Oct 28, 5:11 PM

We could also add AVX2 but I can't test it.

livingsilver94 edited the summary of this revision. (Show Details)Mon, Oct 28, 5:15 PM

Ah, right, this package is also overriding some CFLAGS. Do you want me to patch this behavior?

Depends if the overridden CFLAGS are providing value. Benchmark with and without them overridden, and, maybe list what CFLAGS are being overriden as well.

Never mind. The only flag overridden is -O3 in place of -O2. Using -O2 gives a gain in speed of -0.4109393691590064%, which of course means it's performing worse.

well that's fine. Maybe add - speed to the optimize flags to make it clear that 03 is being used.

well that's fine. Maybe add - speed to the optimize flags to make it clear that 03 is being used.

Allow me to disagree. This could lead a packager to think that by removing - speed argon2 would use -O2, which is not true. I think that a simple comment would be better.

Can we not make a big deal of this please? 0.5% is barely perceptile in keepasx and we aren't optimizing php to run on a web server, so it won't kill you there either. Also, if LTO makes this take even 50% longer to compile, it's a waste of your time and mine.

It's 3% and it's takings seconds to build.
make 18.87s user 2.84s system 81% cpu 26.558 total that's the whole make command for the Solus package.

JoshStrobl requested changes to this revision.Fri, Nov 1, 10:08 AM
JoshStrobl added a subscriber: JoshStrobl.

My only issue with this is the optimize flag isn't set as optimize : lto rather than the list with a single item. Otherwise LGTM.

This revision now requires changes to proceed.Fri, Nov 1, 10:08 AM

I went ahead and tidied up some bits

JoshStrobl requested changes to this revision.Fri, Nov 1, 10:47 AM
JoshStrobl added inline comments.
pspec_x86_64.xml
25

My new glasses could just be screwing with me here, but you may want to check your build log to validate we're actually building and then installing the man file (per your line 22 in package.yml). Because it's not in the pspec.

This revision now requires changes to proceed.Fri, Nov 1, 10:47 AM

Yeah, I temporarily commented the install line and then removed the hash sign, but I forgot to rebuild. Good catch.

livingsilver94 marked an inline comment as done.Fri, Nov 1, 10:51 AM

Add the missing tiny shiny pipe

JoshStrobl accepted this revision.Fri, Nov 1, 11:03 AM

LGTM, thanks!

This revision is now accepted and ready to land.Fri, Nov 1, 11:03 AM
This revision was automatically updated to reflect the committed changes.