builtin: simplify splint_nth methods

[ttytm]

Simplifies the functions for better read and maintainability.

[spytheman]

Have you measured a performance difference?

[ttytm]

Had it tested, though couldn't measure significant differences at 100k iterations. The implementations appear to have the same deviations.

split_nth	rsplit_nth

Is there a potential culprit in the changed code?

[spytheman]

Is there a potential culprit in the changed code?

None, that I could think of. I was just curious, since I observed several surprising results yesterday, while testing c9e6a12 .

[ttytm]

None, that I could think of. I was just curious, since I observed several surprising results yesterday, while testing c9e6a12 .

Oh yeah I see. It really is good to double check. I also had some surprises in the recent days where I had the same thoughts as you put into yet another PR

That will have to wait, until we have a bit better tooling [...]

[spytheman]

I am measuring with this program:

import os
import benchmark

const a = '1,2,3'
const b = '1::2::3'
const c = 'ABCDEF'
const d = ','
const e = ',,,0,,,,,a,,b,'
const f = '1:2:3'
const g = '123'
const h = ''
const line = 'CMD=eprintln(phase=1)'
const s1 = 'volt/twitch.v:34'
const s2 = '2018-01-01z13:01:02'
const s3 = '4627a862c3dec29fb3182a06b8965e0025759e18___1530207969___blue'
const s4 = 'lalala'
const s5 = 'awesome'
const s6 = 'wavy turquoise bags'
const sa1 = 'ABC'
const sa2 = ''
const sa3 = ' '
const sa4 = '  '
const sa5 = 'Ciao come stai? '
const sa6 = 'first row\nsecond row'
const so1 = 'home/dir/lang.zip'
const so2 = 'home/dir/lang.ts.dts'
const so3 = 'home/dir'

fn check_split() int {
	mut res := 0
	res += a.split(',').len
	res += b.split('::').len
	res += c.split('').len
	res += d.split(',').len
	res += e.split(',,').len
	res += s1.split(':').len
	res += s2.split('z').len
	res += s3.split('___').len
	res += s4.split('a').len
	res += s5.split('').len
	res += s6.split(' bags').len
	return res
}

fn check_rsplit() int {
	mut res := 0
	res += s1.split(':').len
	res += s2.split('z').len
	res += s3.split('___').len
	res += s4.split('a').len
	res += s5.split('').len
	res += s6.split(' bags').len
	return res
}

fn check_split_nth() int {
	mut res := 0
	res += a.split_nth(',', -1).len
	res += a.split_nth(',', 0).len
	res += a.split_nth(',', 1).len
	res += a.split_nth(',', 2).len
	res += a.split_nth(',', 10).len
	res += b.split_nth('::', -1).len
	res += b.split_nth('::', 0).len
	res += b.split_nth('::', 1).len
	res += b.split_nth('::', 2).len
	res += b.split_nth('::', 10).len
	res += c.split_nth('', 3).len
	res += c.split_nth('BC', -1).len
	res += d.split_nth('', 3).len
	res += d.split_nth(',', -1).len
	res += d.split_nth(',', 3).len
	res += e.split_nth(',,', 3).len
	res += e.split_nth(',', -1).len
	res += e.split_nth(',', 3).len
	res += f.split_nth(':', 2).len
	res += g.split_nth('', 2).len
	res += h.split_nth('', 2).len
	res += line.split_nth('=', 0).len
	res += line.split_nth('=', 1).len
	res += line.split_nth('=', 2).len
	res += line.split_nth('=', 3).len
	res += line.split_nth('=', 4).len
	return res
}

fn check_rsplit_nth() int {
	mut res := 0
	res += a.rsplit_nth(',', -1).len
	res += a.rsplit_nth(',', 0).len
	res += a.rsplit_nth(',', 1).len
	res += a.rsplit_nth(',', 2).len
	res += a.rsplit_nth(',', 10).len
	res += b.rsplit('::').len
	res += b.rsplit_nth('::', -1).len
	res += b.rsplit_nth('::', 0).len
	res += b.rsplit_nth('::', 1).len
	res += b.rsplit_nth('::', 2).len
	res += b.rsplit_nth('::', 10).len
	res += c.rsplit('').len
	res += c.rsplit_nth('', 3).len
	res += c.rsplit_nth('BC', -1).len
	res += d.rsplit(',').len
	res += d.rsplit_nth('', 3).len
	res += d.rsplit_nth(',', -1).len
	res += d.rsplit_nth(',', 3).len
	res += e.rsplit(',,').len
	res += e.rsplit_nth(',,', 3).len
	res += e.rsplit_nth(',', -1).len
	res += e.rsplit_nth(',', 3).len
	res += f.rsplit_nth(':', 2).len
	res += g.rsplit_nth('', 2).len
	res += a.rsplit(',').len
	res += h.rsplit_nth('', 2).len
	res += line.rsplit_nth('=', 0).len
	res += line.rsplit_nth('=', 1).len
	res += line.rsplit_nth('=', 2).len
	res += line.rsplit_nth('=', 3).len
	res += line.rsplit_nth('=', 4).len
	return res
}

fn check_split_any() int {
	mut res := 0
	res += sa1.split_any('').len
	res += sa2.split_any(' ').len
	res += sa3.split_any(' ').len
	res += sa4.split_any(' ').len
	res += sa5.split_any(' ').len
	res += sa5.split_any('+*').len
	res += sa5.split_any('+* ').len
	res += sa6.split_any(' \n').len
	return res
}

fn check_rsplit_any() int {
	mut res := 0
	res += sa1.rsplit_any('').len
	res += sa2.rsplit_any(' ').len
	res += sa3.rsplit_any(' ').len
	res += sa4.rsplit_any(' ').len
	res += sa5.rsplit_any(' ').len
	res += sa5.rsplit_any('+*').len
	res += sa5.rsplit_any('+* ').len
	res += sa6.rsplit_any(' \n').len
	return res
}

fn check_split_once() int {
	mut res := 0
	path1, ext1 := so1.split_once('.') or { '', '' }
	res += path1.len
	res += ext1.len
	path2, ext2 := so2.split_once('.') or { '', '' }
	res += path2.len
	res += ext2.len
	path3, ext3 := so3.split_once('.') or { '', '' }
	res += path3.len
	res += ext3.len
	return res
}

fn check_rsplit_once() int {
	mut res := 0
	path1, ext1 := so1.rsplit_once('.') or { '', '' }
	res += path1.len
	res += ext1.len
	path2, ext2 := so2.rsplit_once('.') or { '', '' }
	res += path2.len
	res += ext2.len
	path3, ext3 := so3.rsplit_once('.') or { '', '' }
	res += path3.len
	res += ext3.len
	return res
}

const bench_fn_map = {
	'check_split':       check_split
	'check_rsplit':      check_rsplit
	'check_split_nth':   check_split_nth
	'check_rsplit_nth':  check_rsplit_nth
	'check_split_any':   check_split_any
	'check_rsplit_any':  check_rsplit_any
	'check_split_once':  check_split_once
	'check_rsplit_once': check_rsplit_once
}

const max_iterations = os.getenv_opt('MAX_ITERATIONS') or {'100_000'}.int()
dump(max_iterations)
for fname, f in bench_fn_map {
	mut bench := benchmark.start()
	mut sum := i64(0)
	for _ in 0..max_iterations {
		sum += i64(f())
	}
	bench.measure('f: ${fname:-20s} | sum: ${sum:6}')
}

Compiled with:
v -o bench_splitting_strings_new bench_splitting_strings.v on the branch of the PR, and with
v -o bench_splitting_strings_old bench_splitting_strings.v on master.

The results are:

old	new

Compiled with -prod:
v -o bench_splitting_strings_new_prod -prod -cc gcc bench_splitting_strings.v on the branch of the PR, and with
v -o bench_splitting_strings_old_prod -prod -cc gcc bench_splitting_strings.v on master.
The results are:

old with -prod	new with -prod

[spytheman]

(the program above is produced by mechanically transforming a bit the existing tests for the functions + adding a runner to make it easier to run with different iteration counts)

[spytheman]

I think that the difference is that the new variant does more allocations somewhere in s.split and s.rsplit, but I do not know where yet, since:

(the above are results for compilation with -prealloc, i.e. with the allocator, that does not free anything at all)

[spytheman]

I think it is due to this construct:

- is_delim := i + delim.len <= s.len && s.substr(i, i + delim.len) == delim

vs

+ if s[i..i + delim.len] or { break } == delim {

[spytheman]

The old one generated:

bool is_delim = (int)(i + delim.len) <= s.len && 
                    string__eq(string_substr(s, i, (int)(i + delim.len)), delim);

The new one generated:

_result_string _t5 = string_substr_with_check(s, i, (int)(i + delim.len));
if (_t5.is_error) {
    IError err = _t5.err;
    break;
}

if (string__eq(*(string*)&_t5.data, delim)) {

[spytheman]

Yep:

with:

diff --git vlib/builtin/string.v vlib/builtin/string.v
index bca5674e3..c684d7bc2 100644
--- vlib/builtin/string.v
+++ vlib/builtin/string.v
@@ -953,7 +953,7 @@ pub fn (s string) split_nth(delim string, nth int) []string {
 			mut start := 0
 			// Add up to `nth` segments left of every occurrence of the delimiter.
 			for i := 0; i <= s.len; i++ {
-				if s[i..i + delim.len] or { break } == delim {
+				if i + delim.len <= s.len && s[i..i + delim.len] == delim {
 					if nth > 0 && res.len == nth - 1 {
 						break
 					}

[spytheman]

Adding also this patch, speeds up things too (by reducing allocations):

diff --git vlib/builtin/string.v vlib/builtin/string.v
index bca5674e3..9cd997cce 100644
--- vlib/builtin/string.v
+++ vlib/builtin/string.v
@@ -927,7 +927,7 @@ pub fn (s string) split_nth(delim string, nth int) []string {
 		0 {
 			for i, ch in s {
 				if nth > 0 && res.len == nth - 1 {
-					res << s[i..]
+					res << unsafe { s.substr_unsafe(i, s.len - 1) }
 					break
 				}
 				res << ch.ascii_str()
@@ -941,7 +941,7 @@ pub fn (s string) split_nth(delim string, nth int) []string {
 					if nth > 0 && res.len == nth - 1 {
 						break
 					}
-					res << s.substr(start, i)
+					res << unsafe { s.substr_unsafe(start, i) }
 					start = i + 1
 				}
 			}
@@ -953,18 +953,18 @@ pub fn (s string) split_nth(delim string, nth int) []string {
 			mut start := 0
 			// Add up to `nth` segments left of every occurrence of the delimiter.
 			for i := 0; i <= s.len; i++ {
-				if s[i..i + delim.len] or { break } == delim {
+				if i + delim.len <= s.len && unsafe { s.substr_unsafe(i, i + delim.len) } == delim {
 					if nth > 0 && res.len == nth - 1 {
 						break
 					}
-					res << s.substr(start, i)
+					res << unsafe { s.substr_unsafe(start, i) }
 					i += delim.len
 					start = i
 				}
 			}
 			// Then add the remaining part of the string as the last segment.
 			if nth < 1 || res.len < nth {
-				res << s[start..]
+				res << unsafe { s.substr_unsafe(start, s.len - 1) }
 			}
 		}
 	}
@@ -984,7 +984,7 @@ pub fn (s string) rsplit_nth(delim string, nth int) []string {
 		0 {
 			for i := s.len - 1; i >= 0; i-- {
 				if nth > 0 && res.len == nth - 1 {
-					res << s[..i + 1]
+					res << unsafe { s.substr_unsafe(0, i + 1) }
 					break
 				}
 				res << s[i].ascii_str()
@@ -998,29 +998,30 @@ pub fn (s string) rsplit_nth(delim string, nth int) []string {
 					if nth > 0 && res.len == nth - 1 {
 						break
 					}
-					res << s[i + 1..rbound]
+					res << unsafe { s.substr_unsafe(i + 1, rbound) }
 					rbound = i
 				}
 			}
 			if nth < 1 || res.len < nth {
-				res << s[..rbound]
+				res << unsafe { s.substr_unsafe(0, rbound) }
 			}
 		}
 		else {
 			mut rbound := s.len
 			for i := s.len - 1; i >= 0; i-- {
-				is_delim := i - delim.len >= 0 && s[i - delim.len..i] == delim
+				is_delim := i - delim.len >= 0
+					&& unsafe { s.substr_unsafe(i - delim.len, i) } == delim
 				if is_delim {
 					if nth > 0 && res.len == nth - 1 {
 						break
 					}
-					res << s[i..rbound]
+					res << unsafe { s.substr_unsafe(i, rbound) }
 					i -= delim.len
 					rbound = i
 				}
 			}
 			if nth < 1 || res.len < nth {
-				res << s[..rbound]
+				res << unsafe { s.substr_unsafe(0, rbound) }
 			}
 		}
 	}

but it may be out of scope for this PR, given its title.

[ttytm]

Awesome analysis, thanks for sharing the results in that way!

Please be free to push the changes.

[spytheman]

Please be free to push the changes.

Thanks, I will in a moment 🙇🏻‍♂️ .

[spytheman]

The a << s[start..end] -> a << unsafe{ s.substr_unsafe(start,end) } thing, seems general enough, to be done automatically by the compiler in the transformer stage 🤔 .

The intermediate string will not be used for anything else, and will be copied anyway by the << .