Skip to content

reflect: remove calling mapiterkey and mapiterelem #69416

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

Closed
xiaost opened this issue Sep 12, 2024 · 5 comments
Closed

reflect: remove calling mapiterkey and mapiterelem #69416

xiaost opened this issue Sep 12, 2024 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@xiaost
Copy link
Contributor

xiaost commented Sep 12, 2024

Hi, Go team.

I'm working on optimizing iterating over map for an internal data encoding project.
And in our case, I found that one of the bottlenecks is reflect.MapIter .Next.
It can be improved by removing calling mapiterkey.

coz we have already copiedhiter struct from runtime for performance concern.
To make it further, we can use key and elem of hiter struct directly without calling mapiterkey and mapiterelem

This mainly impacts the Next method of reflect.MapIter.
I ran the BenchmarkMapIterNext benchmark in reflect/benchmark_test.go, and here is the output:

goos: darwin
goarch: arm64
pkg: reflect
cpu: Apple M2 Pro
               │  ./old.txt  │              ./new.txt              │
               │   sec/op    │   sec/op     vs base                │
MapIterNext-12   61.44n ± 1%   54.51n ± 1%  -11.29% (p=0.000 n=10)

I would like to raise CR if you're OK with it.

@gopherbot gopherbot added this to the Proposal milestone Sep 12, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@earthboundkid
Copy link
Contributor

In general, performance improvements with no user visible effect can just be submitted. A proposal doesn't need to be approved first unless it's doing something unusual to get the performance.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/612616 mentions this issue: reflect: remove calling mapiterkey, mapiterelem

@ianlancetaylor ianlancetaylor changed the title proposal: reflect: remove calling mapiterkey and mapiterelem reflect: remove calling mapiterkey and mapiterelem Sep 12, 2024
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Sep 12, 2024
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Sep 12, 2024
@ianlancetaylor
Copy link
Member

Taking this out of the proposal process.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 12, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/613395 mentions this issue: reflect: allow inlining mapiterkey/mapiterelem

@mknyszek mknyszek moved this to All-But-Submitted in Go Compiler / Runtime Sep 18, 2024
@github-project-automation github-project-automation bot moved this from All-But-Submitted to Done in Go Compiler / Runtime Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

5 participants