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

runtime: remove concatstring{2,3,4,5} #65020

Open
lasiar opened this issue Jan 8, 2024 · 3 comments · May be fixed by #65028
Open

runtime: remove concatstring{2,3,4,5} #65020

lasiar opened this issue Jan 8, 2024 · 3 comments · May be fixed by #65028
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@lasiar
Copy link

lasiar commented Jan 8, 2024

Background

Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs (accordingly written in c).
After these functions have been replaced to Golang code by commit 61dca94, functions only calls runtime.concatstrings() and that is it.

Proposal

Remove runtime.concatstring{2,3,4,5} and use only runtime.concatstrings(*tmpBuf, []string) string.

API changes

Nope

Benefits

  1. The code is clearer.
  2. Performance:
Benchmark code
package main

import "testing"

func concat2(a, b string) string {
	return a + b
}

func concat3(a, b, c string) string {
	return a + b + c
}

func concat4(a, b, c, d string) string {
	return a + b + c + d
}

func concat5(a, b, c, d, e string) string {
	return a + b + c + d + e
}

func concat6(a, b, c, d, e, f string) string {
	return a + b + c + d + e + f
}

var a = "foo"
var _b = "bar"
var c = "baz"
var d = "ipsum"
var e = "lorem"
var f = "dolor"

func BenchmarkConcat2(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = concat2(a, _b)
	}
}

func BenchmarkConcat3(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = concat3(a, _b, c)
	}
}

func BenchmarkConcat4(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = concat4(a, _b, c, e)
	}
}

func BenchmarkConcat5(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = concat5(a, _b, c, e, d)
	}
}

func BenchmarkConcat6(b *testing.B) {
	for i := 0; i < b.N; i++ {
		_ = concat6(a, _b, c, e, d, f)
	}
}
Result of benchmark
name        old time/op  new time/op  delta
Concat2-10  11.3ns ± 1%  10.6ns ± 0%  -6.37%  (p=0.000 n=18+17)
Concat3-10  14.3ns ± 0%  13.6ns ± 3%  -5.26%  (p=0.000 n=18+20)
Concat4-10  18.3ns ± 5%  17.7ns ± 3%  -3.17%  (p=0.000 n=18+20)
Concat5-10  21.1ns ± 1%  20.6ns ± 0%  -2.26%  (p=0.000 n=18+18)
Concat6-10  23.2ns ± 2%  23.3ns ± 0%    ~     (p=0.084 n=18+19)

The Concat6 must show same result, because changes doesn't affect.

@gopherbot gopherbot added this to the Proposal milestone Jan 8, 2024
lasiar added a commit to lasiar/go that referenced this issue Jan 8, 2024
Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs. But now these are not needed anymore.

Refs golang#65020
lasiar added a commit to lasiar/go that referenced this issue Jan 8, 2024
Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs. But now these are not needed anymore.

Refs golang#65020
lasiar added a commit to lasiar/go that referenced this issue Jan 8, 2024
Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs. But now these are not needed anymore.

Refs golang#65020
lasiar added a commit to lasiar/go that referenced this issue Jan 8, 2024
Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs. But now these are not needed anymore.

Fixes golang#65020
@lasiar lasiar linked a pull request Jan 8, 2024 that will close this issue
@gopherbot
Copy link

Change https://go.dev/cl/554835 mentions this issue: runtime: remove concatstring{2,3,4,5}

@randall77
Copy link
Contributor

Taking out of proposal process, no API changes.

@randall77 randall77 modified the milestones: Proposal, Go1.23 Jan 8, 2024
@randall77 randall77 changed the title proposal: runtime: remove concatstring{2,3,4,5} runtime: remove concatstring{2,3,4,5} Jan 8, 2024
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Jan 9, 2024
lasiar added a commit to lasiar/go that referenced this issue Jan 15, 2024
Functions runtime.concatstring{2,3,4,5} were added by commit: 24699fb to avoid using C varargs. But now these are not needed anymore.

Fixes golang#65020
@CAFxX
Copy link
Contributor

CAFxX commented Feb 15, 2024

See #27801 though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Development

Successfully merging a pull request may close this issue.

5 participants