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

cmd/compile: avoid convT2E for T = struct{} (empty struct) #18402

Closed
rogpeppe opened this Issue Dec 21, 2016 · 11 comments

Comments

Projects
None yet
8 participants
@rogpeppe
Contributor

rogpeppe commented Dec 21, 2016

go version devel +0b2daa5 Thu Dec 1 11:23:17 2016 +0000 linux/amd64

An empty struct is held in an interface value as a non-nil pointer. This means that
converting an empty struct to an interface is more expensive that
it needs to be be - it involves a call to convT2E.

Given that conversion of empty structs to interfaces is common (for example,
empty structs are often used as context keys) it may be better to use
the nil pointer as the value part of the interface, making
interface{}(struct{}{}) as efficient as interface{}((*int)(nil)).

@minux

This comment has been minimized.

Show comment
Hide comment
@minux

minux Dec 21, 2016

Member
Member

minux commented Dec 21, 2016

@rogpeppe

This comment has been minimized.

Show comment
Hide comment
@rogpeppe

rogpeppe Dec 21, 2016

Contributor

But the current suggested way for context keys is to
save it into a variable of type interface{} to reduce
the cost of interface conversion?

Why should there be any cost to interface conversion of a struct{} ?
By contrast, there's no cost to converting a pointer to an interface and converting
a known-nil pointer to an interface is even cheaper than copying an
interface AFAICS.

ISTM that the suggested workaround should be unnecessary.
It imposes cognitive overhead because you need to invent two
names rather than one, and because by defining it as an interface
variable, you open up the possibility of it being modified.

FWIW the "current suggested way" as seen in https://blog.golang.org/context is
to use a typed integer constant, which is definitely not ideal as it will incur
an allocation every time it's used.

Contributor

rogpeppe commented Dec 21, 2016

But the current suggested way for context keys is to
save it into a variable of type interface{} to reduce
the cost of interface conversion?

Why should there be any cost to interface conversion of a struct{} ?
By contrast, there's no cost to converting a pointer to an interface and converting
a known-nil pointer to an interface is even cheaper than copying an
interface AFAICS.

ISTM that the suggested workaround should be unnecessary.
It imposes cognitive overhead because you need to invent two
names rather than one, and because by defining it as an interface
variable, you open up the possibility of it being modified.

FWIW the "current suggested way" as seen in https://blog.golang.org/context is
to use a typed integer constant, which is definitely not ideal as it will incur
an allocation every time it's used.

@rogpeppe

This comment has been minimized.

Show comment
Hide comment
@rogpeppe

rogpeppe Dec 21, 2016

Contributor

BTW I think the compiler should probably completely optimize out all "struct{}"-valued variables. Using such a variable could be exactly the same as using a struct{}{} literal. Currently different code is generated for:

interface{}(struct{}{})

and

 var x struct{}
 ...
     interface{}(x)

I don't see any reason that there should be a difference.

That should perhaps be considered another issue though.

Contributor

rogpeppe commented Dec 21, 2016

BTW I think the compiler should probably completely optimize out all "struct{}"-valued variables. Using such a variable could be exactly the same as using a struct{}{} literal. Currently different code is generated for:

interface{}(struct{}{})

and

 var x struct{}
 ...
     interface{}(x)

I don't see any reason that there should be a difference.

That should perhaps be considered another issue though.

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Dec 21, 2016

Contributor

See also #17725.
We can certainly avoid the convT2E cost in this case. We could use &zerobase (what malloc returns for zero-sized allocations) instead of nil (not sure if it matters, have to think about it).

type contextKey struct{}
var SomethingKey interface{} = contextKey{}

This is a general way to optimize doing the interface construction only once. Optimizing struct{} won't invalidate that whole pattern. But it does become useless for struct{}. I'm ok with that.

Contributor

randall77 commented Dec 21, 2016

See also #17725.
We can certainly avoid the convT2E cost in this case. We could use &zerobase (what malloc returns for zero-sized allocations) instead of nil (not sure if it matters, have to think about it).

type contextKey struct{}
var SomethingKey interface{} = contextKey{}

This is a general way to optimize doing the interface construction only once. Optimizing struct{} won't invalidate that whole pattern. But it does become useless for struct{}. I'm ok with that.

@rogpeppe

This comment has been minimized.

Show comment
Hide comment
@rogpeppe

rogpeppe Dec 21, 2016

Contributor

We could use &zerobase (what malloc returns for zero-sized allocations) instead of nil

That already seems to be the case. The pointer value in interface{}(struct{}{}) is the same as &struct{}{}.

Contributor

rogpeppe commented Dec 21, 2016

We could use &zerobase (what malloc returns for zero-sized allocations) instead of nil

That already seems to be the case. The pointer value in interface{}(struct{}{}) is the same as &struct{}{}.

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Dec 21, 2016

Contributor

Right. But we don't need to call convT2E, which calls mallocgc, which then returns &zerobase. We can just use &zerobase directly.

Contributor

randall77 commented Dec 21, 2016

Right. But we don't need to call convT2E, which calls mallocgc, which then returns &zerobase. We can just use &zerobase directly.

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Dec 21, 2016

Contributor
Contributor

randall77 commented Dec 21, 2016

@cstockton

This comment has been minimized.

Show comment
Hide comment
@cstockton

cstockton Dec 21, 2016

@randall77 I deleted the post after I made it, I was thinking the conversation was headed around higher level assignment semantics, this makes sense I think:

Right. But we don't need to call convT2E, which calls mallocgc, which then returns &zerobase. We can just use &zerobase directly.

cstockton commented Dec 21, 2016

@randall77 I deleted the post after I made it, I was thinking the conversation was headed around higher level assignment semantics, this makes sense I think:

Right. But we don't need to call convT2E, which calls mallocgc, which then returns &zerobase. We can just use &zerobase directly.

@iand

This comment has been minimized.

Show comment
Hide comment
@iand

iand Dec 27, 2016

Contributor

As an additional data point golint checks basic data types for suitability as context keys and explicitly OK's struct{} (golang/lint#245)

Contributor

iand commented Dec 27, 2016

As an additional data point golint checks basic data types for suitability as context keys and explicitly OK's struct{} (golang/lint#245)

@rsc rsc added this to the Go1.9Early milestone Jan 4, 2017

@rsc rsc changed the title from cmd/compile: more efficient empty struct representation in interfaces to cmd/compile: avoid convT2E for T = struct{} (empty struct) Jan 4, 2017

@josharian josharian self-assigned this Jan 22, 2017

@josharian

This comment has been minimized.

Show comment
Hide comment
@josharian

josharian Jan 22, 2017

Contributor

Gophetbot still seems ill, so: CL 35562

Contributor

josharian commented Jan 22, 2017

Gophetbot still seems ill, so: CL 35562

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Jan 24, 2017

CL https://golang.org/cl/35562 mentions this issue.

@gopherbot gopherbot closed this in 8958d8c Feb 2, 2017

@golang golang locked and limited conversation to collaborators Feb 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.