cmd/compile: We can avoid extra mapaccess in map[k] = append(m[k],...) #24364
Comments
|
I am going to provide fix and create pull request. I will take in account all comments and changes made in scope of #23661 that is connected. |
|
Just realized that more likely optimization is going to have the following restrictions: It works namely for case: m[k] = append(m[k], ...) where we can tell that right and left parts are exactly the same during Order. It will not be able to work:
2 and 3 are feasible but they are so rare that I don't want to add specific optimization. I am testing implementation now and added BenchmarkMapAppendAssign: Run it before and after |
|
Also need to take care with channel ops and things that can panic. See func samesafeexpr. |
|
Change https://golang.org/cl/100838 mentions this issue: |
|
Somewhat related: #5147 (#5147 (comment) etc) |
|
I checked that after fix mapaccess is not present in asm code: |
|
@vkuzmin-uber, you can add a code generation tests to your CL. They are pretty easy to add nowadays. |
|
@bradfitz Brad, could you please point me to some example with "code generation tests"? I think such test is pretty critical for change like that. It's too easy to add regression accidentally. (We just found, for instance, that optimization doesn't cover particular types with "mapslow") |
|
If you look in test/codegen, there are a bunch of tests for the generated
assembly code. Read the README, and take a look at movesmall.go, it has
tests to make sure that we don't call memmove in certain situations, which
is similar to the test you want.
…On Fri, Mar 16, 2018 at 12:16 PM, vkuzmin-uber ***@***.***> wrote:
@bradfitz <https://github.com/bradfitz> Brad, could you please point me
to some example with "code generation tests"? I think such test is pretty
critical for change like that. It's too easy to add regression
accidentally. (We just found, for instance, that optimization doesn't cover
particular types with "mapslow")
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#24364 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGkgIKbKpU08Zce91QxE_0nKYHgIiJDlks5tfA-CgaJpZM4Sn3IV>
.
|
|
Thanks, I provided mapaccess codegen test. I also wrote small article https://kuzminva.wordpress.com/2018/03/18/146-coverage-codegen-tests/ |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?go 1.9, go 1.10
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env)?darwin_amd64
What did you do?
Looked at ASM code of:
map[k] = append(map[k],...)
If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
What did you expect to see?
Namely in this case mapaccess(m, k) can be avoided, see optimization #23661 that was merged. I intentionally separated it and willing to provide fix for "map + append" case.
What did you see instead?
see 2 calls
func mapaccess*(..) unsafe.Pointer
func mapassign*(...) unsafe.Pointer
The text was updated successfully, but these errors were encountered: