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.

Sign up for GitHub

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: devirtualization of interface calls with type assertions #64824

Open
mateusz834 opened this issue Dec 20, 2023 · 8 comments · May be fixed by #71711 or #71935
Open

cmd/compile: devirtualization of interface calls with type assertions #64824

mateusz834 opened this issue Dec 20, 2023 · 8 comments · May be fixed by #71711 or #71935
Assignees
@mdempsky @mateusz834
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Backlog

Comments

@mateusz834
Copy link
Member

Consider:

h := sha256.New() // escapes to heap
h.(encoding.BinaryUnmarshaler).UnmarshalBinary(d) // not devirtualized
h.Write(d) // devirtualized

https://go.dev/play/p/JYPsrebi5Z5

h escapes to heap, because the compiler does not take the opportunity to devirtualize the UnmarshalBinary call.

Same thing happens with even a simpler case (hash.Hash always implements io.Writer)

h := sha256.New()
h.Write(d) // devirtualized
h.(io.Writer).Write(d) // not devirtualized
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 20, 2023
@mateusz834 mateusz834 added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 20, 2023
@mateusz834 mateusz834 added this to the Backlog milestone Dec 20, 2023
@thanm
Copy link
Contributor

thanm commented Dec 20, 2023

@mdempsky

@smiletrl
Copy link
Contributor 

h.(encoding.BinaryUnmarshaler).UnmarshalBinary(d) // not devirtualized
h.(io.Writer).Write(d) // not devirtualized

It seems to me that encoding.BinaryUnmarshaler and io.Writer are not concrete types, which is why they are not devirtualized ^

@mateusz834
Copy link
Member Author

It seems to me that encoding.BinaryUnmarshaler and io.Writer are not concrete types

This is the whole point of of devirtualization. Same h := sha256.New() is also an interface and h.Write() call is devirtualized.

@mdempsky mdempsky self-assigned this Dec 24, 2023
@mdempsky
Copy link
Contributor

This seems doable for Go 1.23. Did this come up in real world code though? For example, h.(io.Writer).Write seems unrealistic when you can just write h.Write.

@mateusz834
Copy link
Member Author

mateusz834 commented Dec 24, 2023 •
edited
Loading

Did this come up in real world code though?

Only the first example, but this is not performance related in my case, I just was curious why the hash escaped and it turned out to this.

@mknyszek mknyszek added the FeatureRequest Issues asking for a new feature that does not need a proposal. label Jan 3, 2024
@mateusz834 mateusz834 mentioned this issue Oct 30, 2024
Open
@FiloSottile
Copy link
Contributor

Support for this would influence API design in #69521 (comment).

@mateusz834 mateusz834 linked a pull request Feb 13, 2025 that will close this issue
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/649195 mentions this issue: cmd/compile: devirtualize interface calls with type assertions

@mateusz834 mateusz834 self-assigned this Feb 14, 2025
@mateusz834 mateusz834 linked a pull request Feb 24, 2025 that will close this issue
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/652036 mentions this issue: cmd/compile/internal/devirtualize: improve concrete type analysis

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

@mdempsky mdempsky

@mateusz834 mateusz834

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
7 participants
@mdempsky @FiloSottile @mknyszek @smiletrl @gopherbot @thanm @mateusz834