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

reflect: New hits a RWMutex for types with no ptrToThis field #17931

Closed
crawshaw opened this issue Nov 15, 2016 · 3 comments
Closed

reflect: New hits a RWMutex for types with no ptrToThis field #17931

crawshaw opened this issue Nov 15, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@crawshaw
Copy link
Member

In Go 1.7 I attempted to remove the ptrToThis field from reflect.rtype. It caused regressions, so I brought it back.

However I didn't completely bring it back. We used to include a ptrToThis value for any type where the pointer is used in the final program. Now we only include it for a subset of types (named values with methods).

This means a greater set of types hit a slow path in reflect.New, which slows down packages like the protocol buffer decoder.

A fix is to partially revert CL 20287 and CL 19695.

@crawshaw crawshaw added this to the Go1.8 milestone Nov 15, 2016
@crawshaw crawshaw self-assigned this Nov 15, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 16, 2016
@bradfitz
Copy link
Contributor

David, what's the status here?

/cc @ianlancetaylor @rsc

@crawshaw
Copy link
Member Author

CL 33411 by @ianlancetaylor resolves this. I have a partial CL re-introducing weak symbols that I could get going, but I prefer the general approach in 33411.

@ianlancetaylor
Copy link
Contributor

As I mentioned to David already, I think weak symbols are always a win, but that CL 33411 is only sometimes a win. And CL 33411 is not for 1.8 anyhow. So I think we do want weak symbols.

@golang golang locked and limited conversation to collaborators Nov 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants