expr: Optimizing for integer values

[Arp-1]

An attempt at resolving issue #5591

[sylvestre]

nice, did you do some benchmark ? - just curious on what kind of impact this can have
(hyperfine is pretty good for this)

[Arp-1]

Not yet, was looking if there are some open test data sets available for expr. Will look into hyperfine, thanks for your suggestion!

[tertsdiepraam]

Great! I'm also curious to see the performance impact. I have a few small suggestions, and one case where I think you've accidentally changed the behaviour.

[Arp-1]

Sorry for getting back on this so late, had a busy week!
@tertsdiepraam thanks for your review, have made fixes for all your comments.
I did some quick & dirty benchmarking on my local system (terrible reliability, got inaccuracies of ~30%) with hyperfine. For small numerical expressions( < 10 operations), I saw almost no gains (5% at best). For bigger numerical expressions (~100 operations) with huge numbers (< 10^150), saw an improvement of around 40%. For string operations, There was a performance loss of no more than 5%.
Tested release build of expr with shell=none - main branch (8b1a82e) against my branch (Arp-1@4d2ae84)
Let me know if I should put more details of my testing setup.

[tertsdiepraam]

Nice work! I think you took my advice with From a bit too far, but hopefully what I meant is clear now 😄 It is looking better though!

Also those numbers for the performance are super interesting. I wonder why the string operations are slower! Did you try with --release?

[Arp-1]

Thanks for taking a look at it again @tertsdiepraam !
On your benchmarking question, yes, I'd built expr with --release. Though reiterating on the instability of the runtimes on my local system, the spread of time taken for optimized expr was 0.74ms to 2.23ms and for unoptimized expr it was 0.73 ms to 2.85ms:

Maybe I should try and do this on a little less noisy system :)

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/tail/inotify-dir-recreate. tests/tail/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[tertsdiepraam]

Excellent! Just one final suggestion. Great benchmarks! I think it makes sense that it's noisy because it's not the biggest improvement, but it is a great change nonetheless!

[tertsdiepraam]

I still think this one is weird. Maybe it could be eval_as_usize? Or we could just inline it:

Suggested change

	let pos: usize = Option::<usize>::from(pos.eval()?).unwrap_or(0);
	let length: usize = Option::<usize>::from(length.eval()?).unwrap_or(0);
	let pos = pos.eval()?.eval_as_bigint().map_or(0, |n| n.to_usize());
	let length = pos.eval()?.eval_as_bigint().map_or(0, |n| n.to_usize());

[Arp-1]

done

[tertsdiepraam]

I made a small change (see the last commit), but this looks ready to be merged once the CI is green. Thanks!

[Arp-1]

This is great! Thanks so much for all your help.

[sylvestre]

green, merged :)