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

cmd/compile: numberLines SSA pass does not handle inlining well #29279

Open
josharian opened this Issue Dec 15, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@josharian
Copy link
Contributor

josharian commented Dec 15, 2018

Consider func rewriteBlockAMD64. It spans lines 66518-69626 of rewriteAMD64.go. However, it calls (at line 66699) func log2, located at lines 369-370 of rewrite.go. log2 is inlined. Thus when the numberLines pass looks for the first and last lines of func rewriteBlockAMD64, it doesn't find lines 66518-69626, it finds lines 369-69626! It then allocates a giant sparsemap to accomodate all these lines.

This is currently causing toolspeed pain; allocating those sparsemaps accounts for ~17% of the allocated space when compiling the ssa package. (The ssa package has many long files with many functions that inline functions with small line numbers.) Related: #27739.

(This also makes me nervous about whether we're correctly handling one function that inlines another when both functions happen to occupy the same line numbers, but in different files. I assume @dr2chase can speak to that without having to go spelunking.)

I don't know what the correct fix is, but we remove the implicit assumption that the line numbers involved in a function are dense.

Marking as Go 1.12 to match #27739, but it seems to me that we can probably push to 1.13.

@josharian josharian added this to the Go1.12 milestone Dec 15, 2018

@ysmolsky ysmolsky pinned this issue Dec 17, 2018

@ysmolsky ysmolsky unpinned this issue Dec 17, 2018

@dr2chase

This comment has been minimized.

Copy link
Contributor

dr2chase commented Dec 17, 2018

I would be nervous about inlining line numbers matching exactly, I can check to see if that is done right but I am not 100% sure it is.

I'll see about redoing the sparse line numbering tables.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 17, 2018

Change https://golang.org/cl/154617 mentions this issue: cmd/compile: index line number tables by source file to improve sparsity

@aclements

This comment has been minimized.

Copy link
Member

aclements commented Dec 18, 2018

Pushing to 1.13. Feel free to push back.

@aclements aclements modified the milestones: Go1.12, Go1.13 Dec 18, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 28, 2019

Change https://golang.org/cl/169722 mentions this issue: cmd/compile: Use slice of biasedsparsemaps instead of map

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.