[go] errors: faster Error method for *joinError
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
errors: faster Error method for *joinError
Assert len(e.errs) > 0, remove i > 0 in the loop.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: ff105bcf0351a13ec80f9ecbf746dc842083ea23
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/errors/join.go b/src/errors/join.go
index 1c486d5..c1c890d 100644
--- a/src/errors/join.go
+++ b/src/errors/join.go
@@ -38,11 +38,14 @@
}
func (e *joinError) Error() string {
- var b []byte
- for i, err := range e.errs {
- if i > 0 {
- b = append(b, '\n')
- }
+ // Assert len(e.errs) > 0.
+ if len(e.errs) == 1 {
+ return e.errs[0].Error()
+ }
+
+ b := []byte(e.errs[0].Error())
+ for _, err := range e.errs[1:] {
+ b = append(b, '\n')
b = append(b, err.Error()...)
}
return string(b)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
2 comments:
Patchset:
Thanks, but I don't think this is worth doing. Is there even a measurable performance difference for the normal case of a small number of errors?
File src/errors/join.go:
Patch Set #1, Line 41:
// Assert len(e.errs) > 0.Why is this a good idea? Why not just do something reasonable for len(e.errs) == 0, as the current code does?
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Ian Lance Taylor.
1 comment:
File src/errors/join.go:
Patch Set #1, Line 41:
// Assert len(e.errs) > 0.
Why is this a good idea? Why not just do something reasonable for len(e. […]
"Join returns nil if every value in errs is nil."
The comment above is from errors.Join.
So the return of errors.Join has two cases:
1. nil, the string representation is "<nil>".
2. a *joinError, the "errs" in it must not be empty.It's unnecessary to handle the empty errs of e, even though it seems unreasonable.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Sense As.
1 comment:
File src/errors/join.go:
Patch Set #1, Line 41:
// Assert len(e.errs) > 0.
"Join returns nil if every value in errs is nil." […]
OK, thanks, but I still don't think this is worth doing.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil.
Patch set 1:Code-Review +1
Damien Neil (Gerrit)
Patchset:
Thanks, but I don't think this is worth doing. […]
There will be one less allocation for the case of one error, where this avoids copying that error's text. I'd expect the difference in that case to be measurable, although I don't know if it's worth optimizing for.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Ian Lance Taylor.
Patch set 1:-Code-Review
1 comment:
Patchset:
There will be one less allocation for the case of one error, where this avoids copying that error's […]
For the case of one error, I did a simple test. This error is created from a string of length 1024.
```text
goos: darwin
goarch: arm64
Benchmark_Old_Error-8 5531764 208.1 ns/op 2048 B/op 2 allocs/op
Benchmark_New_Error-8 510634675 2.181 ns/op 0 B/op 0 allocs/op
```
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Ian Lance Taylor.
1 comment:
Patchset:
I believe it's correct, running fast, and allocating less memory in some case. And I also believe that your wisdom has discovered it before me. What I don't understand is what prevents such a change?
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
1 comment:
File src/errors/join.go:
Patch Set #1, Line 41:
// Assert len(e.errs) > 0.
OK, thanks, but I still don't think this is worth doing.
I'll withdraw my objection. Someone else should review. (And this will have to wait for 1.22). Thanks.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #2 to this change.
errors: faster Error method for *joinError
Assert len(e.errs) > 0, remove i > 0 in the loop.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 67cae65f6e32005281368339bc9f0d1957537fb8
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 16 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #3 to this change.
errors: faster Error method for *joinError
Since "Join returns nil if every value in errs is nil",
"errs" in a *joinError is certainly non-empty.
Determine whether the length of e.errs is 1, and if so,
return the error string. This will cause one less allocation.
Iterating from index 1 of e.errs, "if i > 0" can be removed.
It's safe to use unsafe.String to avoid allocation when
converting byte slice to string, since "b" has at least one byte '\n'.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 67cae65f6e32005281368339bc9f0d1957537fb8
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 16 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #4 to this change.
errors: faster Error method for *joinError
Since "Join returns nil if every value in errs is nil",
"errs" in a *joinError is certainly non-empty.
Determine whether the length of e.errs is 1, and if so,
return the error string. This will cause one less allocation.
Iterating from index 1 of e.errs, "if i > 0" can be removed.
It's safe to use unsafe.String to avoid allocation when
converting byte slice to string, since "b" has at least one byte '\n'.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 2da1acbb2d86aae78c8a4072c676b3d1ad044ec9
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 16 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #5 to this change.
errors: optimize the Error method of *joinError for faster execution and less allocation
Since "Join returns nil if every value in errs is nil",
"errs" in a *joinError is certainly non-empty.
Determine whether the length of e.errs is 1, and if so,
return the error string. This will cause one less allocation.
Iterating from index 1 of e.errs, "if i > 0" can be removed.
It's safe to use unsafe.String to avoid allocation when
converting byte slice to string, since "b" has at least one byte '\n'.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 2da1acbb2d86aae78c8a4072c676b3d1ad044ec9
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 16 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #6 to this change.
errors: optimize the Error method of *joinError for faster execution and less allocation
Since "Join returns nil if every value in errs is nil",
"errs" in a *joinError is certainly non-empty.
Determine whether the length of e.errs is 1, and if so,
return the error string. This will cause one less allocation.
Iterating from index 1 of e.errs, "if i > 0" can be removed.
It's safe to use unsafe.String to avoid allocation when
converting byte slice to string, since "b" has at least one byte '\n'.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 3e0e1517291318ca12d36c8678bba2214e637486
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 32 insertions(+), 5 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #7 to this change.
errors: optimize the Error method of *joinError for faster execution and less allocation
Precompute capacity of the return string, preallocate byte slice to reduce memory allocation.
Add panic when Join output length is overflow.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 3e0e1517291318ca12d36c8678bba2214e637486
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 32 insertions(+), 5 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #8 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Precompute capacity of the return string, preallocate byte slice to reduce memory allocation.
Add panic when Join output length is overflow.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 3e0e1517291318ca12d36c8678bba2214e637486
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 32 insertions(+), 5 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
1 comment:
Patchset:
Thanks. The current Error method has certain risks: when the capacity of the returned string exceeds the bounds, it will cause a memory leak (although the possibility is very small). I don't know if this is worth fixing right away.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Gerrit Bot uploaded patch set #9 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Compute the length of the returned string to preallocate byte slice for reducing memory allocation, it will panic when the length is overflow.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: 3e0e1517291318ca12d36c8678bba2214e637486
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 32 insertions(+), 5 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
1 comment:
Patchset:
If the risk does exist, can I request a freeze exception to review?
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
1 comment:
Patchset:
Thanks. […]
I'm sorry, I don't understand the risk here. It seems to me that the bound on string capacity is so large that a program that exceeds them will crash because it is unable to allocate memory.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
1 comment:
Patchset:
Sorry, I mean: It is a huge waste of time and memory because it cannot fail immediately.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
1 comment:
Patchset:
Sorry, I mean: It is a huge waste of time and memory because it cannot fail immediately.
Thanks. I don't think that problem is significant enough to justify a freeze exception. It's hard to believe that any real program generates that many errors in practice.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Daniel Nephin, Sense As.
2 comments:
File src/errors/join.go:
Patch Set #9, Line 60:
nstr := len(err.Error())This doubles the number of `Error` calls.
It's not obvious to me that the tradeoff here is a good one; it certainly won't be universally correct.
Patch Set #9, Line 62:
panic("errors: Join output length overflow")If we were doing this, a simpler and cleaner approach would be to skip preallocating the slice on overflow and let the runtime handle the panic.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #10 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Compute the length of the returned string to preallocate byte slice for reducing memory allocation, it will panic when the length is overflow.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: f7f9b6f3d01eb249e25bbbf624ee5118b29cc3df
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 23 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin, Sense As.
Gerrit Bot uploaded patch set #11 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Handle the cases of len(e.errs) < 2 at the very beginning.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: e9ffff493adab03d65721f560cf58297fa03da9b
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 22 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
3 comments:
Patchset:
Thank you so much!
File src/errors/join.go:
Patch Set #9, Line 60:
nstr := len(err.Error())
This doubles the number of `Error` calls. […]
Done
Patch Set #9, Line 62:
panic("errors: Join output length overflow")
If we were doing this, a simpler and cleaner approach would be to skip preallocating the slice on ov […]
Done
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Daniel Nephin.
3 comments:
File src/errors/join.go:
Since this is just an optimization for the len==1 case, it can be
```
if len(e.errs) == 1 {
return e.errs[0].Error()
}
```Patch Set #11, Line 56:
// the slice on overflow and let the runtime handle the panic.Stale comment: We never preallocate the slice.
Patch Set #11, Line 63:
// TODO: replace with unsafe.String(&b[0], len(b))Is there a reason this is a TODO rather than doing it now?
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Gerrit Bot uploaded patch set #12 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Handle the cases of len(e.errs) < 2 at the very beginning.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: ed8003bfbcae8efd42e54895db0554c139b9d3a7
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 15 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Since this is just an optimization for the len==1 case, it can be […]
Done
Patch Set #11, Line 56:
// the slice on overflow and let the runtime handle the panic.Stale comment: We never preallocate the slice.
Done
Patch Set #11, Line 63:
// TODO: replace with unsafe.String(&b[0], len(b))Is there a reason this is a TODO rather than doing it now?
Done
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Gerrit Bot uploaded patch set #13 to this change.
errors: optimize *joinError's Error method for less allocation and faster execution
Handle the case of one error at the beginning.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: ed8003bfbcae8efd42e54895db0554c139b9d3a7
GitHub-Pull-Request: golang/go#60026
---
M src/errors/join.go
1 file changed, 15 insertions(+), 6 deletions(-)
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Sense As (Gerrit)
Attention is currently required from: Daniel Nephin.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Daniel Nephin.
Patch set 13:Run-TryBot +1
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
1 comment:
Patchset:
Seems the commit's git parent is old, same as my another CL. Could you please help me rebase it? Thank you so much!
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Patch set 14:Run-TryBot +1
Sense As (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Damien Neil, Daniel Nephin.
Patch set 14:Code-Review +1
Damien Neil (Gerrit)
Damien Neil submitted this change.
13 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
errors: optimize *joinError's Error method for less allocation and faster execution
Handle the case of one error at the beginning.
Use unsafe.String to avoid memory allocation when converting byte slice to string.
Change-Id: Ib23576f72b1d87489e6f17762be483f62ca4998a
GitHub-Last-Rev: ed8003bfbcae8efd42e54895db0554c139b9d3a7
GitHub-Pull-Request: golang/go#60026
Reviewed-on: https://go-review.googlesource.com/c/go/+/493237
Reviewed-by: Damien Neil <dn...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: David Chase <drc...@google.com>
Run-TryBot: Ian Lance Taylor <ia...@golang.org>
---
M src/errors/join.go
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/errors/join.go b/src/errors/join.go
index 1c486d5..349fc06 100644
--- a/src/errors/join.go
+++ b/src/errors/join.go
@@ -4,6 +4,10 @@
package errors
+import (
+ "unsafe"
+)
+
// Join returns an error that wraps the given errors.
// Any nil error values are discarded.
// Join returns nil if every value in errs is nil.
@@ -38,14 +42,19 @@
}
func (e *joinError) Error() string {
- var b []byte
- for i, err := range e.errs {
- if i > 0 {
- b = append(b, '\n')
- }
+ // Since Join returns nil if every value in errs is nil,
+ // e.errs cannot be empty.
+ if len(e.errs) == 1 {
+ return e.errs[0].Error()
+ }
+
+ b := []byte(e.errs[0].Error())
+ for _, err := range e.errs[1:] {
+ b = append(b, '\n')
b = append(b, err.Error()...)
}
- return string(b)
+ // At this point, b has at least one byte '\n'.
+ return unsafe.String(&b[0], len(b))
}
func (e *joinError) Unwrap() []error {
To view, visit change 493237. To unsubscribe, or for help writing mail filters, visit settings.