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

Jump to bottom

cmd/compile: lay out global errors.New results in static data #30820

Closed
rsc opened this issue Mar 14, 2019 · 6 comments
Closed

cmd/compile: lay out global errors.New results in static data #30820

rsc opened this issue Mar 14, 2019 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.20

Comments

@rsc
Copy link
Contributor

rsc commented Mar 14, 2019

A significant fraction of init time in some programs
(notably the go command but surely others) is spent
executing lines like:

var ErrMyError = errors.New("my error")

This is such a widespread use that it may well be
worth recognizing in the compiler and turning into
static data, so that it can be laid out in the data section,
discarded by the linker as appropriate, and have no
link-time overhead at all.

The specific pattern would be exactly the above
(var foo = errors.New(constString)) and it would turn into

var foo = error(&errors.errorString{s: constString})

This would require that the compiler be able to lay out an
implicit interface conversion in the data section as well.
If that can't be done in general, it would suffice to special
case the "errors.errorString to error" conversion.

If this sounds like fun to someone familiar with the
compiler, please have a look. Thanks.
@rsc rsc added this to the Go1.13 milestone Mar 14, 2019
@randall77
Copy link
Contributor

The compiler already does the latter part of what you described. Starting with:

type errorString struct {
	s string
}

func (e *errorString) Error() string {
	return e.s
}

var foo = error(&errorString{s: "foobar"})

The compiler generates:

"".foo SDATA size=16
	0x0000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
	rel 0+8 t=1 go.itab.*"".errorString,error+0
	rel 8+8 t=1 ""..stmp_0+0
""..stmp_0 SDATA size=16
	0x0000 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00  ................
	rel 0+8 t=1 go.string."foobar"+0
go.string."foobar" SRODATA dupok size=6
	0x0000 66 6f 6f 62 61 72                                foobar

@mvdan
Copy link
Member

mvdan commented Mar 14, 2019

This would be a nice follow-up step to #30468.

/cc @bcmills @neild @mpvl

@mvdan
Copy link
Member

mvdan commented Mar 14, 2019

I wonder, would there be a plan to do the same for simple global uses of fmt.Errorf like var ErrFoo = fmt.Errorf("foo error")?

Not that fmt.Errorf is necessarily better for global errors, but i f we made errors.New comparatively inexpensive, it could lead to a recommended practice to never use fmt.Errorf for globals.

Which is maybe fine, but ideally we don't want developers to have to remember that distinction.

@mvdan mvdan mentioned this issue Mar 14, 2019
Closed
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@dpinela dpinela mentioned this issue Jun 10, 2020
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@gopherbot
Copy link

Change https://go.dev/cl/426156 mentions this issue: cmd/compile: lay out global errors.New results in static data

@gopherbot
Copy link

Change https://go.dev/cl/450136 mentions this issue: cmd/compile: handle simple inlined calls in staticinit

@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Nov 16, 2022
@gopherbot
Copy link

Change https://go.dev/cl/452817 mentions this issue: cmd/compile: reenable inlstaticinit

gopherbot pushed a commit that referenced this issue Nov 23, 2022
@rsc @gopherbot
cmd/compile: reenable inlstaticinit
f89b39c 
This was disabled in CL 452676 out of an abundance of caution,
but further analysis has shown that the failures were not being
caused by this optimization. Instead the sequence of commits was:

CL 450136 cmd/compile: handle simple inlined calls in staticinit
...
CL 449937 archive/tar, archive/zip: return ErrInsecurePath for unsafe paths
...
CL 451555 cmd/compile: fix static init for inlined calls

The failures in question became compile failures in the first CL
and started building again after the last CL.
But in the interim the code had been broken by the middle CL.
CL 451555 was just the first time that the tests could run and fail.

For #30820.

Change-Id: I65064032355b56fdb43d9731be2f9f32ef6ee600
Reviewed-on: https://go-review.googlesource.com/c/go/+/452817
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
8 participants
@rsc @andybons @dmitshur @mvdan @ALTree @bcmills @randall77 @gopherbot