expr: Optimizing for integer values by Arp-1 · Pull Request #5614 · uutils/coreutils

Arp-1 · 2023-12-03T10:22:35Z

An attempt at resolving issue #5591

sylvestre · 2023-12-03T11:11:13Z

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

Arp-1 · 2023-12-03T11:25:10Z

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.

src/uu/expr/src/syntax_tree.rs

Arp-1 · 2023-12-09T07:06:11Z

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?

src/uu/expr/src/syntax_tree.rs

Arp-1 · 2023-12-10T20:58:07Z

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 · 2023-12-10T21:09:57Z

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 · 2023-12-11T09:31:07Z

src/uu/expr/src/syntax_tree.rs

+                let pos: usize = Option::<usize>::from(pos.eval()?).unwrap_or(0);
+                let length: usize = Option::<usize>::from(length.eval()?).unwrap_or(0);


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());

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 · 2023-12-14T17:48:58Z

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

sylvestre · 2023-12-14T17:50:55Z

green, merged :)

Optimize expr for numerical values

117ab77

tertsdiepraam requested changes Dec 3, 2023

View reviewed changes

Arp-1 added 6 commits December 3, 2023 15:09

Formatting fixes in expr

d8a64a9

code and styling fixes in expr

f8573d5

Fix errors

5672e3d

Fix lint issues in expr

21c041f

Refactoring for lint issues

9ecd6a2

impl from trait instead of into

4d2ae84

tertsdiepraam reviewed Dec 9, 2023

View reviewed changes

src/uu/expr/src/syntax_tree.rs Outdated Show resolved Hide resolved

src/uu/expr/src/syntax_tree.rs Outdated Show resolved Hide resolved

src/uu/expr/src/syntax_tree.rs Outdated Show resolved Hide resolved

Arp-1 added 3 commits December 11, 2023 01:19

Merge branch 'main' into feat-refactor-expr

17f2b83

review fixes

fa0c64d

style lint fixes

824371d

tertsdiepraam reviewed Dec 11, 2023

View reviewed changes

Arp-1 and others added 2 commits December 11, 2023 20:47

remove from trait for NumOrStr

3bf966d

expr: clean up conversion from bigint to usize

7f23faf

tertsdiepraam approved these changes Dec 14, 2023

View reviewed changes

sylvestre merged commit f248cc6 into uutils:main Dec 14, 2023

Arp-1 deleted the feat-refactor-expr branch December 14, 2023 18:06

BenWiederhake mentioned this pull request Apr 1, 2024

expr: optimization for integer values #5591

Closed

		let pos: usize = Option::<usize>::from(pos.eval()?).unwrap_or(0);
		let length: usize = Option::<usize>::from(length.eval()?).unwrap_or(0);

Uh oh!

Conversation

Arp-1 commented Dec 3, 2023

Uh oh!

sylvestre commented Dec 3, 2023

Uh oh!

Arp-1 commented Dec 3, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arp-1 commented Dec 9, 2023

Uh oh!

tertsdiepraam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arp-1 commented Dec 10, 2023

Uh oh!

github-actions bot commented Dec 10, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

Arp-1 Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Arp-1 commented Dec 14, 2023

Uh oh!

sylvestre commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tertsdiepraam left a comment •

edited

Loading