Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Use pointers so plan.Clean() works #1804

Merged
merged 1 commit into from
Apr 27, 2020
Merged

Conversation

shanson7
Copy link
Collaborator

Verified locally. Results in a moderate reduction in alloc rate for query nodes.

@Dieterbe
Copy link
Contributor

fix #1799

I must be missing something, but given that a map is basically a by-reference type, copying the plan (and getting a "reference" to the same map) should be harmless. what am i missing?

@shanson7
Copy link
Collaborator Author

We are updating the wrong reference.

As an example:
We start with plan A with A.data at address 0x100 pointing to map at address 0x50
When calling executePlan we copy that plan to plan B with B.data at 0x150 pointing to address 0x50
When calling plan.Run() as a value ref we copy B as plan C with C.data at 0x200 pointing to address 0x50.
We update C.data so 0x200 now points to our dataMap at 0x175. However A.data and B.data both still point to 0x50 (the wrong map).

@Dieterbe
Copy link
Contributor

Ah, I see. I thought we just inserted into the map, but we re-assign to it in plan.Run with p.dataMap = dataMap. d'oh...

@Dieterbe Dieterbe merged commit 75884f2 into grafana:master Apr 27, 2020
@shanson7 shanson7 deleted the fix_plan_pool branch April 27, 2020 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants