[
https://issues.apache.org/jira/browse/KUDU-3197?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17211340#comment-17211340
]
Andrew Wong edited comment on KUDU-3197 at 10/9/20, 8:16 PM:
-------------------------------------------------------------
You're right in that the scanner creates a schema to represent its projection.
However, the underlying iterators may take references to the current schema
while iterating, the tablet service might take references while preparing to
scan, etc. While most if not all of these accesses are short-lived, we need to
be careful not to destruct the schemas while these references still exist.
Grepping around to audit current usages (with some false positives for copies
and log messages):
{code:java}
~/Repositories/kudu/src/kudu > grep -r "meta.*[^_]schema()" .
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet_metadata.cc: if (!(*metadata)->schema().Equals(schema)) {
./tablet/tablet_metadata.cc: "match expected schema ($1)",
(*metadata)->schema().ToString(),
./tablet/diff_scan-test.cc: SchemaBuilder
builder(tablet->metadata()->schema());
./tablet/tablet_replica-test.cc:
ASSERT_OK(SchemaToPB(SchemaBuilder(tablet()->metadata()->schema()).Build(),
&orig_schema_pb));
./tablet/tablet_replica-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet_replica-test.cc:
ASSERT_OK(SchemaToPB(SchemaBuilder(tablet()->metadata()->schema()).Build(),
&orig_schema_pb));
./tablet/tablet_replica-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet.cc: : key_schema_(metadata->schema().CreateKeyProjection()),
./tablet/tablet.cc: metadata_->SetSchema(*op_state->schema(),
op_state->schema_version());
./tablet/tablet.cc: RollingDiskRowSetWriter drsw(metadata_.get(),
merge->schema(), DefaultBloomSizing(),
./tablet/rowset_metadata.h: return tablet_metadata_->schema();
./tablet/tablet.h: return &metadata_->schema();
./tablet/all_types-scan-correctness-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tools/kudu-tool-test.cc: .PartitionDebugString(meta->partition(),
meta->schema());
./tools/kudu-tool-test.cc: debug_str = meta->schema().ToString();
./tools/kudu-tool-test.cc: .PartitionDebugString(meta->partition(),
meta->schema());
./tools/kudu-tool-test.cc: debug_str = meta->schema().ToString();
./tools/tool_action_local_replica.cc: const auto& col_idx =
meta->schema().find_column_by_id(col_id);
./tools/tool_action_local_replica.cc:
meta->schema().column(col_idx).name() : "?");
./tools/tool_action_local_replica.cc: Schema schema = meta->schema();
./tools/tool_action_local_replica.cc: const Schema& schema = meta->schema();
./tools/tool_action_local_replica.cc:
meta->schema())
./tserver/tablet_service.cc: Schema tablet_schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc:
CHECK_OK(SchemaToPB(replica->tablet_metadata()->schema(),
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: Schema tablet_schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const Schema& tablet_schema =
replica->tablet_metadata()->schema();
./tserver/scanners.cc:
spec().lower_bound_key()->Stringify(tablet_metadata->schema()))));
./tserver/scanners.cc:
spec().exclusive_upper_bound_key()->Stringify(tablet_metadata->schema()))));
./tserver/tserver_path_handlers.cc:
tmeta->schema());
./tserver/tserver_path_handlers.cc: const Schema& schema = tmeta->schema();
./master/sys_catalog.cc: if (!metadata->schema().Equals(BuildTableSchema())) {
./master/sys_catalog.cc: return(Status::Corruption("Unexpected schema",
metadata->schema().ToString()));
./client/scan_token-internal.cc:
RETURN_NOT_OK(SchemaFromPB(metadata.schema(), &schema));
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema(); {code}
and
{code:java}
~/Repositories/kudu/src/kudu > grep -r "tablet_schema()" .
./tablet/cfile_set.h: const Schema &tablet_schema() const { return
rowset_metadata_->tablet_schema(); }
./tablet/diskrowset.cc: const Schema* schema =
&rowset_metadata_->tablet_schema();
./tablet/diskrowset.cc:
RETURN_NOT_OK(NewCompactionInput(&rowset_metadata_->tablet_schema(),
./tablet/cfile_set.cc: int key_col_id = tablet_schema().column_id(0);
./tablet/cfile_set.cc: key_schema_for_vlog =
base_data_->tablet_schema().CreateKeyProjection();
./tablet/rowset_metadata.h: const Schema& tablet_schema() const {
./tools/tool_action_local_replica.cc: Schema key_proj =
rs_meta->tablet_schema().CreateKeyProjection(); {code}
As for the two approaches you mentioned, #1 would be to wrap the current
{{Schema}} class with a {{std::shared_ptr}, while #2 would extend {{Schema}}}}
with {{RefCountedThreadSafe}} and use {{scoped_refptr}} everywhere (both of
these approaches are outlined here
[https://kudu.apache.org/docs/contributing.html#_pointers]).
I share your performance concerns, given some of these references are taken on
the hot path. I'll need to dig in a bit more to understand how prominently
cycles from a {{simple_spinlock}} would show up here.
was (Author: andrew.wong):
You're right in that the scanner creates a schema to represent its projection.
However, the underlying iterators may take references to the current schema
while iterating, the tablet service might take references while preparing to
scan, etc. While most if not all of these accesses are short-lived, we need to
be careful not to destruct the schemas while these references still exist.
Grepping around to audit current usages (with some false positives for copies
and log messages):
{code:java}
~/Repositories/kudu/src/kudu > grep -r "meta.*[^_]schema()" .
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet-schema-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet_metadata.cc: if (!(*metadata)->schema().Equals(schema)) {
./tablet/tablet_metadata.cc: "match expected schema ($1)",
(*metadata)->schema().ToString(),
./tablet/diff_scan-test.cc: SchemaBuilder
builder(tablet->metadata()->schema());
./tablet/tablet_replica-test.cc:
ASSERT_OK(SchemaToPB(SchemaBuilder(tablet()->metadata()->schema()).Build(),
&orig_schema_pb));
./tablet/tablet_replica-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet_replica-test.cc:
ASSERT_OK(SchemaToPB(SchemaBuilder(tablet()->metadata()->schema()).Build(),
&orig_schema_pb));
./tablet/tablet_replica-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tablet/tablet.cc: : key_schema_(metadata->schema().CreateKeyProjection()),
./tablet/tablet.cc: metadata_->SetSchema(*op_state->schema(),
op_state->schema_version());
./tablet/tablet.cc: RollingDiskRowSetWriter drsw(metadata_.get(),
merge->schema(), DefaultBloomSizing(),
./tablet/rowset_metadata.h: return tablet_metadata_->schema();
./tablet/tablet.h: return &metadata_->schema();
./tablet/all_types-scan-correctness-test.cc: SchemaBuilder
builder(tablet()->metadata()->schema());
./tools/kudu-tool-test.cc: .PartitionDebugString(meta->partition(),
meta->schema());
./tools/kudu-tool-test.cc: debug_str = meta->schema().ToString();
./tools/kudu-tool-test.cc: .PartitionDebugString(meta->partition(),
meta->schema());
./tools/kudu-tool-test.cc: debug_str = meta->schema().ToString();
./tools/tool_action_local_replica.cc: const auto& col_idx =
meta->schema().find_column_by_id(col_id);
./tools/tool_action_local_replica.cc:
meta->schema().column(col_idx).name() : "?");
./tools/tool_action_local_replica.cc: Schema schema = meta->schema();
./tools/tool_action_local_replica.cc: const Schema& schema = meta->schema();
./tools/tool_action_local_replica.cc:
meta->schema())
./tserver/tablet_service.cc: Schema tablet_schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc:
CHECK_OK(SchemaToPB(replica->tablet_metadata()->schema(),
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: Schema tablet_schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const auto& schema =
replica->tablet_metadata()->schema();
./tserver/tablet_service.cc: const Schema& tablet_schema =
replica->tablet_metadata()->schema();
./tserver/scanners.cc:
spec().lower_bound_key()->Stringify(tablet_metadata->schema()))));
./tserver/scanners.cc:
spec().exclusive_upper_bound_key()->Stringify(tablet_metadata->schema()))));
./tserver/tserver_path_handlers.cc:
tmeta->schema());
./tserver/tserver_path_handlers.cc: const Schema& schema = tmeta->schema();
./master/sys_catalog.cc: if (!metadata->schema().Equals(BuildTableSchema())) {
./master/sys_catalog.cc: return(Status::Corruption("Unexpected schema",
metadata->schema().ToString()));
./client/scan_token-internal.cc:
RETURN_NOT_OK(SchemaFromPB(metadata.schema(), &schema));
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema();
./client/client-test.cc: Schema schema =
tablet_replica->tablet()->metadata()->schema(); {code}
and
{code:java}
~/Repositories/kudu/src/kudu > grep -r "tablet_schema()" .
./tablet/cfile_set.h: const Schema &tablet_schema() const { return
rowset_metadata_->tablet_schema(); }
./tablet/diskrowset.cc: const Schema* schema =
&rowset_metadata_->tablet_schema();
./tablet/diskrowset.cc:
RETURN_NOT_OK(NewCompactionInput(&rowset_metadata_->tablet_schema(),
./tablet/cfile_set.cc: int key_col_id = tablet_schema().column_id(0);
./tablet/cfile_set.cc: key_schema_for_vlog =
base_data_->tablet_schema().CreateKeyProjection();
./tablet/rowset_metadata.h: const Schema& tablet_schema() const {
./tools/tool_action_local_replica.cc: Schema key_proj =
rs_meta->tablet_schema().CreateKeyProjection(); {code}
As for the two approaches you mentioned, #1 would be to wrap the current
{{Schema}} class with a {{std::shared_ptr}, while #2 would extend {{Schema}}
with {{RefCountedThreadSafe}} and use {{scoped_refptr}} everywhere (both of
these approaches are outlined here
https://kudu.apache.org/docs/contributing.html#_pointers).
I share your performance concerns, given some of these references are taken on
the hot path. I'll need to dig in a bit more to understand how prominently
cycles from a {{simple_spinlock}} would show up here.
> Tablet keeps all history schemas in memory may result in high memory
> consumption
> --------------------------------------------------------------------------------
>
> Key: KUDU-3197
> URL: https://issues.apache.org/jira/browse/KUDU-3197
> Project: Kudu
> Issue Type: Improvement
> Components: tablet
> Affects Versions: 1.12.0
> Reporter: wangningito
> Assignee: wangningito
> Priority: Minor
> Attachments: image-2020-09-25-14-45-33-402.png,
> image-2020-09-25-14-49-30-913.png, image-2020-09-25-15-05-44-948.png
>
>
> In case of high frequency of updating table, memory consumption of
> kudu-tserver may be very high, and the memory in not tracked in the memory
> page.
> This is the memory usage of a tablet, the memory consumption of tablet-xxx‘s
> peak is 3.6G， but none of its' childrens' memory can reach.
> !image-2020-09-25-14-45-33-402.png!
> So I use pprof to get the heap sampling. The tserver started for long but the
> memory is still consuming by TabletBootstrap:PlayAlterSchemaRequest.
> !image-2020-09-25-14-49-30-913.png!
> I change the `old_schemas_` in tablet_metadata.h to a fixed size vector,
> // Previous values of 'schema_'.
> // These are currently kept alive forever, under the assumption that
> // a given tablet won't have thousands of "alter table" calls.
> // They are kept alive so that callers of schema() don't need to
> // worry about reference counting or locking.
> std::vector<Schema*> old_schemas_;
> The heap sampling then becomes
> !image-2020-09-25-15-05-44-948.png!
> So, to make application layer more flexible, it could be better to make the
> size of the old_schemas configurable.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
