-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
proposal: reflect: add Named() bool to Type interface #27149
Comments
Here's a possible alternative - somehow help the compiler realise that we just care about
|
One counter-point to the JSON case is that we could instead do the reflect work ahead of time for structs; see #16212. Still, this seems like a common reflection operation that should be way cheaper. And reflection is used in many more places than just |
Is adding |
@as yes, because Type has unexported methods. |
While I understand that adding |
@CAFxX do you have implementation ideas of this partial inlining? I've tried several changes to make Go inliner more "dynamic" and make decisions with some caller context. It made some benchmarks faster, some slower. This might be the reason why we still have current cost model, for example. It's trivial to make it worse, but hard to make it universally better (it may actually require profile-guided optimization, since guessing statically is non-trivial). |
@CAFxX of course it would be best if this was fast with no changes to the API - but the hard problem is figuring out how that would work. Make the compiler smarter? Special-case the reflect package in the compiler? Somehow rewrite the method to do less work? I haven't come up with a way to do it, and I'm not even sure it's possible, hence this proposal. |
<3% in one case does not seem worth the extra API here. |
Sorry, I don't know what you're referring to.
I opened this proposal precisely because I didn't see an easy way to do this :) I could make the reflect struct heavier to then cache the name string, but I don't know if that's a tradeoff we want to make. |
@rsc sorry to comment on a frozen issue, but I'm still confused about the arguments to close this proposal. Like I said in the comment above, I don't see a way to make I completely forgot to follow up on this and the thread got locked. Unlocking for now. |
Additional context: @mvdan asked if it was alright to unlock and follow up. I said yes. |
If we think it's worth it, and if it's faster, we could use a The current We could implement partial inlining as suggested above. The encoding/json code should check There are quite a few areas we could explore rather than adding new API. New API seems like a lot, and the performance improvement doesn't seem like all that much in the larger scheme of things. |
Thanks, @ianlancetaylor. Next time I'm in the mood for some encoding/json performance tuning I'll try your ideas and report back. I'm not sure why I hadn't noticed your reply until now, I must have read the notification and forgotten about it. |
Right now, the only way to tell if a
reflect.Type
is a named type is to dot.Name() != ""
. This is quite common:However, the function implementation is quite complex:
I don't think it would ever be possible for the compiler to know that we just care about the first branch, since it's possible that
t.String
would return a string likefoo.
, in which cases[i+1:]
at the end would also be the empty string.I propose that we add a method like
Named() bool
to the interface, implemented like:A call to this function would be inlineable and not require string handling, so it would be massively cheaper than the original
t.Name() != ""
. As proof, here's what happens if we implement it and use it in JSON decoding:One can imagine that the
gob
andxml
packages would also see many-percent speedups, on top of the reflect package itself getting faster.I realise that this is a case of adding extra API just for the sake of performance. However, I fail to see another way to cheaply see if a
reflect.Type
is named, which is a common operation that should be trivial. If anyone has any ideas on how to makeName() != ""
just as fast somehow, please do speak up.On the plus side, there aren't any other flag bits that would be interesting to export, so this shouldn't lead to adding a countless number of boolean methods.
cc @josharian @randall77 @ianlancetaylor @quasilyte
The text was updated successfully, but these errors were encountered: