vlib: simplify byte character conditions by utilizing methods

vlang / v

Simple, fast, safe, compiled language for developing maintainable software. Compiles itself in <1s with zero library dependencies. Supports automatic C => V translation. https://vlang.io

MIT License

35.5k stars 2.15k forks source link

vlib: simplify byte character conditions by utilizing methods #21725

Closed ttytm closed 4 days ago

spytheman commented 4 days ago

What is the performance impact?

yuyi98 commented 4 days ago

Because is_alnum()/is_capital()/is_letter() are inline functions, there is no performance impact and the overall clarity is much clearer.

ttytm commented 4 days ago

Yep, should be only only a cosmetic / read- / maintainability change.

```
(c >= `a` && c <= `z`) || (c >= `A` && c <= `Z`)
```
occurrences of those are replaced with https://github.com/vlang/v/blob/c5c49d36852ff2346d5bbd900cb64d1493635df9/vlib/builtin/string.v#L2065-L2068
```
c >= `0` && c <= `9`
```
replaced with https://github.com/vlang/v/blob/c5c49d36852ff2346d5bbd900cb64d1493635df9/vlib/builtin/string.v#L2037-L2040
```
(c >= `a` && c <= `z`) || (c >= `A` && c <= `Z`) || (c >= `0` && c <= `9`)
```
replaced with https://github.com/vlang/v/blob/c5c49d36852ff2346d5bbd900cb64d1493635df9/vlib/builtin/string.v#L2072-L2075

To not change anything else, e.g. in case of below (in vlib/v/util/scanning.v)

(c >= `a` && c <= `z`) || (c >= `A` && c <= `Z`) || c == `_` || (c >= `0` && c <= `9`)

I'm replacing it with

c.is_letter() || c == `_` || c.is_digit()
// instead of 
c.is_alnum() || c == `_`

spytheman commented 4 days ago

Because is_alnum()/is_capital()/is_letter() are inline functions, there is no performance impact and the overall clarity is much clearer.

In theory I agree, but have you measured, for example, recompiling V itself before/after the change? Keep in mind, that compilers are not perfect in inlining functions, and some of those comparisons, are used in relatively hot loops.

spytheman commented 4 days ago

V executables compiled with -prod -cc gcc-11, are consistently a bit faster with the changes here:

spytheman commented 4 days ago

V executables compiled with just v self (i.e. using tcc), are in contrast a bit slower, but the relative difference is much smaller, and sometimes the new version is faster too:

ttytm commented 4 days ago

Thanks for verifying @spytheman ! Didn't found the a free moment to do and share those tests on time.

JalonSolov commented 3 days ago

I'm glad the extra performance testing was done. Too many times in the past I've seen someone say "It gets inlined, so it'll be fast."

2 things to always keep in mind:

Inline is a suggestion, not a guarantee. The C compiler is free to ignore it.
Inline only works (sometimes, currently, at least) with the C backend. It has no affect on the JavaScript, WASM, native, etc. backends.

However, with the tcc tests, it appears the changes don't have any significant impact, so should be fine all around.