Conversation
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
|
@janisz: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| Subsystem: metrics.CentralSubsystem.String(), | ||
| Name: "store_serialized_bytes_upsert_histogram", | ||
| Help: "Size of serialized column", | ||
| //TODO: adjust to the real usage after tests |
There was a problem hiding this comment.
Please add a JIRA ticket reference (add a ticket for this follow-up fine-tuning if needed to get a ticket number)
| @@ -0,0 +1,82 @@ | |||
| # Process Filter Optimization: String Keys → uint64 Hashes | |||
There was a problem hiding this comment.
This feels unrelated and very much worthy of its own dedicated PR. Please move to separate PR.
| func insertIntoActiveComponents(batch *pgx.Batch, pool pgSearch.BufferPool, obj *storage.ActiveComponent) (*[]byte, error) { | ||
|
|
||
| serialized, marshalErr := obj.MarshalVT() | ||
| buf := pool.Get(obj.SizeVT()) |
There was a problem hiding this comment.
Interesting would be to see if the new Green Tea GC achieves the same performance.
There was a problem hiding this comment.
Yeah, I was wondering how we can measure the difference. For example change to swiss map went unnoticed at least by me
rhybrillou
left a comment
There was a problem hiding this comment.
The change looks good overall.
I'd be protective in the generated code that calls MarshalToSizedBufferVT and default to MarshalVT if the buffer is too small (this might require some default return values for the returned buffer in the insert function as well as specific processing for the returned buffers linked to the edge cases.
There was a problem hiding this comment.
Does this change belong to this PR ?
There was a problem hiding this comment.
I think this change is already on master.
| Subsystem: metrics.CentralSubsystem.String(), | ||
| Name: "store_serialized_bytes_upsert_histogram", | ||
| Help: "Size of serialized column", | ||
| //TODO: adjust to the real usage after tests |
There was a problem hiding this comment.
Please add a JIRA ticket reference (add a ticket for this follow-up fine-tuning if needed to get a ticket number)
| {{if not $schema.Parent }} | ||
| serialized, marshalErr := obj.MarshalVT() | ||
| buf := pool.Get(obj.SizeVT()) | ||
| n, marshalErr := obj.MarshalToSizedBufferVT(*buf) |
There was a problem hiding this comment.
Would it make sense to compare the buffer capacity with obj.SizeVT() ?
It could create problems if we try to serialize to a too small buffer.
| buf := pool.Get(obj.SizeVT()) | ||
| pooledBuffers = append(pooledBuffers, buf) |
There was a problem hiding this comment.
Ensure the buffer is large enough to write the serialized object into.
Description
This PR brings a pooling optimisation for store upsert. Instead of marshaling into a new slice of bytes we can use a pool just like grpc does (and using the same layered pool implementation). This should reduce the pressure on GC as less objects will be to collect.
Refs:
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!