fix: improve query planning time #1326
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
collect fields info along with building tree
devsergiy
force-pushed
the
sergiy/eng-8285-improve-query-planning-time
branch
from
85594c9 to
8cb0c3c
Compare
| ctx := context.Background() | ||
| labels := pprof.Labels("walker_id", w.walkerID) | ||
|
|
||
| pprof.Do(ctx, labels, func(ctx context.Context) { |
There was a problem hiding this comment.
Are we okay with putting this into production?
| walker := astvisitor.WalkerFromPool2("A") | ||
| defer walker.Release() |
There was a problem hiding this comment.
It gets walker from walkerPoolA, but Release will put it back to walkerPool
| func WalkerFromPool2(pool string) *Walker { | ||
| switch pool { | ||
| case "A": | ||
| walker := walkerPoolA.Get() | ||
| if walker == nil { | ||
| w := NewWalkerWithID(8, "RepresentationVariables") | ||
| return &w | ||
| } | ||
| return walker.(*Walker) | ||
| case "B": | ||
| walker := walkerPoolB.Get() | ||
| if walker == nil { | ||
| w := NewWalkerWithID(8, "CollectNodes") | ||
| return &w | ||
| } | ||
| return walker.(*Walker) | ||
| default: | ||
| walker := walkerPool.Get() | ||
| if walker == nil { | ||
| w := NewWalker(8) | ||
| return &w | ||
| } | ||
| return walker.(*Walker) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This function might be doing too much. The only thing that is actually used here is the case "A" branch. Also for the pool we can define how to create a new instance of the object via the New method:
walkerPoolA = sync.Pool{
New: func() any {
return new(NewWalkerWithID(8, "RepresentationVariables"))
},
}So the getting of new items would be as simple as:
walker := walkerPoolA.Get().(*walker.Walker)This might give you ideas how to simplify the managment around pools.
| }, | ||
| DisableResolveFieldPositions: true, | ||
| }, WithDefaultPostProcessor())) | ||
| }, WithDefaultPostProcessor(), WithSkipReason("fix me"))) |
There was a problem hiding this comment.
Do you plan to fix it in this PR?
| } | ||
| } | ||
|
|
||
| type FieldsLimitedVisitor struct { |
There was a problem hiding this comment.
I cannot find where it is being used...
| import ( | ||
| "context" | ||
| "errors" | ||
| "unique" |
There was a problem hiding this comment.
I cannot understand why you have used unique here. Could you point why interning for strings was required?
2 participants
@coderabbitai summary
Checklist