Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builtin: simplify splint_nth methods #21563

Merged
merged 4 commits into from
May 25, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented May 24, 2024

Simplifies the functions for better read and maintainability.

@ttytm ttytm force-pushed the builtin/cleanup-split-nth branch from 5689bad to b30c2e2 Compare May 24, 2024 18:09
vlib/builtin/string.v Outdated Show resolved Hide resolved
@spytheman
Copy link
Member

Have you measured a performance difference?

@ttytm
Copy link
Member Author

ttytm commented May 25, 2024

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

split_nth rsplit_nth
Screenshot_20240525_110646 Screenshot_20240525_111052

Is there a potential culprit in the changed code?

@spytheman
Copy link
Member

spytheman commented May 25, 2024

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
Copy link
Member Author

ttytm commented May 25, 2024

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
Copy link
Member

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
image image

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
image image

@spytheman
Copy link
Member

(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
Copy link
Member

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:
image

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

@spytheman
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Yep:
image

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
Copy link
Member

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.

image

@ttytm
Copy link
Member Author

ttytm commented May 25, 2024

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

Please be free to push the changes.

@spytheman
Copy link
Member

Please be free to push the changes.

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

@spytheman
Copy link
Member

spytheman commented May 25, 2024

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 << .

… fix the new allocation in the loop of split_nth
@spytheman spytheman merged commit 0fdc32e into vlang:master May 25, 2024
69 checks passed
@ttytm ttytm deleted the builtin/cleanup-split-nth branch May 25, 2024 13:52
spytheman added a commit to felipensp/v that referenced this pull request May 27, 2024
* master:
  cgen: fix array fixed initialization on struct from call (vlang#21568)
  testing: implement a separate `-show-asserts` option, for cleaner test output (`-stats` still works, and still shows both the compilation stats and the asserts) (vlang#21578)
  toml: fix `@[toml: ]`, support `@[skip]` (vlang#21571)
  os: fix debugger_present() for non Windows OSes (vlang#21573)
  builtin: simplify splint_nth methods (vlang#21563)
  net.http: change default http.Server listening address to :9009, to avoid conflicts with tools, that start their own http servers on 8080 like bytehound (vlang#21570)
  os: make minior improvement to C function semantics and related code (vlang#21565)
  io: cleanup prefix_and_suffix/1 util function (vlang#21562)
  os: remove mut declarions for unchanged vars in `os_nix.c.v` (vlang#21564)
  builtin: reduce allocations in s.index_kmp/1 and s.replace/2 (vlang#21561)
  ci: shorten path used for unix domain socket tests (to fit in Windows path limits)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants