Avoid unnecessarily incurring the cost of HashWrapper results #755
Conversation
The HashWrapper is very expensive to create. It goes unused unless specifically accessed. Without breaking backwards compatibility, this change allows the HashWrapped response to be available, but not computed for every result by default. This significantly improves the performance of search results in my tests.
|
Please let me know what I can do to fix the checks. I signed the CLA, but I don't see what I can do for CI. |
|
Hi @martinemde, this looks promising, thanks!, I understand that creating Regarding CLA, I can't find you in our records by name -- can you ping me by email, [email protected], so we can sort it out? CI is problematic for some time, I'll run the tests locally. I can't guarantee I'll process the patch in terms of days, but I'll try to look into it. Please ping me again by mentioning me on the ticket if I forget about it. Thanks! |
|
@karmi I updated a bit since I found this exact patch would not solve all our issues. We'd still end up creating the Mash even though we just access the result one time with I'm happy to discuss the exact interface. I'm solving our issue, but I think you'd be a better judge of what the interface should be. Looks like the CLA glitched. I tried again and it went through to my email this time. I guess I missed something the first time. |
| @@ -45,6 +45,14 @@ class MyDocument; end | |||
| assert_equal 5, subject.response._shards.total | |||
| end | |||
|
|
|||
| should "provide access to the raw respanse" do | |||
There was a problem hiding this comment.
There is a typo with respanse instead of response right ?
There was a problem hiding this comment.
@guillaumebriday Thanks for noticing. Fixed.
…out breaking backwards compatibility
|
@martinemde I've pinged you here. Let me know if the pull request helps with the performance. |
|
Closing this, please see my comment here #161 (comment) |
The HashWrapper is very expensive to create. It goes unused unless
specifically accessed. Without breaking backwards compatibility,
this change allows the HashWrapped response to be available, but not
computed for every result by default. This significantly improves
the performance of search results in my tests.
This addresses part of the performance concern in #161 while maintaining backwards compatibility. If you only use the deserialized results, you'll never need to create the HashWrapper. Only if you actually use the response object will you get the wrapped results.
In our app, the creation of a Hashie::Mash was responsible for about 300ms of our 770ms response time. This action only ever used the deserialized result (which is sourced by converting to a Mash, and then back to a Hash) and never the response. Patching this to remove the Mash entirely makes a huge difference in performance.
I hope this can silently help others so they don't need to dig into this particular performance issue.