Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove thread state change callstack feature flag #4256

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Remove thread state change callstack feature flag #4256

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a28ed5
Remove thread state change callstack feature flag
florian-kuebler Oct 4, 2022
a55d14c
review comment
florian-kuebler Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/ClientFlags/ClientFlags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,6 @@ ABSL_FLAG(bool, auto_frame_track, true, "Automatically add the default Frame Tra

ABSL_FLAG(bool, time_range_selection, false, "Enable time range selection feature.");

ABSL_FLAG(bool, tracepoint_callstack_collection, false,
"Enable callstack gathering for tracepoints feature.");

ABSL_FLAG(bool, symbol_store_support, false, "Enable experimental symbol store support.");

// Disables retrieving symbols from the instance. This is intended for symbol store e2e tests.
Expand Down
3 changes: 0 additions & 3 deletions src/ClientFlags/include/ClientFlags/ClientFlags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ ABSL_DECLARE_FLAG(bool, auto_frame_track);
// Enables time range selection feature.
ABSL_DECLARE_FLAG(bool, time_range_selection);

// Enables callstack gathering with tracepoints.
ABSL_DECLARE_FLAG(bool, tracepoint_callstack_collection);

// Enables experimental symbol store support.
ABSL_DECLARE_FLAG(bool, symbol_store_support);

Expand Down
5 changes: 0 additions & 5 deletions src/OrbitQt/CaptureOptionsDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,6 @@ CaptureOptionsDialog::CaptureOptionsDialog(QWidget* parent)
ui_->devModeGroupBox->hide();
ui_->wineNoneRadioButton->hide();
}

if (!absl::GetFlag(FLAGS_tracepoint_callstack_collection)) {
ui_->threadStateChangeCallstackCollectionCheckBox->hide();
ui_->threadStateChangeCallstackMaxCopyRawStackSizeWidget->hide();
}
}

void CaptureOptionsDialog::SetEnableSampling(bool enable_sampling) {
Expand Down
95 changes: 42 additions & 53 deletions src/OrbitQt/orbitmainwindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1169,35 +1169,28 @@ void OrbitMainWindow::LoadCaptureOptionsIntoApp() {
app_->SetEnableApi(settings.value(kEnableApiSettingKey, true).toBool());
app_->SetEnableIntrospection(settings.value(kEnableIntrospectionSettingKey, false).toBool());

if (absl::GetFlag(FLAGS_tracepoint_callstack_collection)) {
CaptureOptions::ThreadStateChangeCallStackCollection const
collect_callstack_on_thread_state_change =
static_cast<CaptureOptions::ThreadStateChangeCallStackCollection>(
settings
.value(kEnableCallStackCollectionOnThreadStateChanges,
orbit_qt::CaptureOptionsDialog::
kThreadStateChangeCallStackCollectionDefaultValue)
.toInt());
if (settings.value(kCollectThreadStatesSettingKey, false).toBool()) {
app_->SetThreadStateChangeCallstackCollection(collect_callstack_on_thread_state_change);
} else {
app_->SetThreadStateChangeCallstackCollection(
CaptureOptions::kThreadStateChangeCallStackCollectionUnspecified);
}

uint16_t stack_dump_size = static_cast<uint16_t>(
settings
.value(
kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeMaxCopyRawStackSizeDefaultValue)
.toUInt());
app_->SetThreadStateChangeCallstackStackDumpSize(stack_dump_size);
CaptureOptions::ThreadStateChangeCallStackCollection const
collect_callstack_on_thread_state_change = static_cast<
CaptureOptions::ThreadStateChangeCallStackCollection>(
settings
.value(
kEnableCallStackCollectionOnThreadStateChanges,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeCallStackCollectionDefaultValue)
.toInt());
if (settings.value(kCollectThreadStatesSettingKey, false).toBool()) {
app_->SetThreadStateChangeCallstackCollection(collect_callstack_on_thread_state_change);
} else {
app_->SetThreadStateChangeCallstackCollection(
CaptureOptions::kThreadStateChangeCallStackCollectionUnspecified);

app_->SetThreadStateChangeCallstackStackDumpSize(std::numeric_limits<uint16_t>::max());
}

uint16_t stack_dump_size = static_cast<uint16_t>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thread_state_change_stack_dump_size to not confuse with the one for sampling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

settings
.value(kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeMaxCopyRawStackSizeDefaultValue)
.toUInt());
app_->SetThreadStateChangeCallstackStackDumpSize(stack_dump_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were setting the stack_dump_size as max before in case of unspecified callstack collection. I guess that this doesn't matter at all in this case, since it will be ignored, but could we have a potential issue because unespecified value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that shouldn't be an issue. That value is ignored in that case anyways.


DynamicInstrumentationMethod instrumentation_method = static_cast<DynamicInstrumentationMethod>(
settings
.value(kDynamicInstrumentationMethodSettingKey,
Expand Down Expand Up @@ -1337,25 +1330,22 @@ void OrbitMainWindow::on_actionCaptureOptions_triggered() {
QVariant::fromValue(orbit_qt::CaptureOptionsDialog::kLocalMarkerDepthDefaultValue))
.toULongLong());

if (absl::GetFlag(FLAGS_tracepoint_callstack_collection)) {
CaptureOptions::ThreadStateChangeCallStackCollection collect_callstack_on_thread_state_change =
static_cast<CaptureOptions::ThreadStateChangeCallStackCollection>(
settings
.value(kEnableCallStackCollectionOnThreadStateChanges,
orbit_qt::CaptureOptionsDialog::
kThreadStateChangeCallStackCollectionDefaultValue)
.toInt());
dialog.SetEnableCallStackCollectionOnThreadStateChanges(
collect_callstack_on_thread_state_change ==
CaptureOptions::kThreadStateChangeCallStackCollection);

dialog.SetThreadStateChangeCallstackMaxCopyRawStackSize(static_cast<uint16_t>(
settings
.value(
kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeMaxCopyRawStackSizeDefaultValue)
.toUInt()));
}
CaptureOptions::ThreadStateChangeCallStackCollection collect_callstack_on_thread_state_change =
static_cast<CaptureOptions::ThreadStateChangeCallStackCollection>(
settings
.value(
kEnableCallStackCollectionOnThreadStateChanges,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeCallStackCollectionDefaultValue)
.toInt());
dialog.SetEnableCallStackCollectionOnThreadStateChanges(
collect_callstack_on_thread_state_change ==
CaptureOptions::kThreadStateChangeCallStackCollection);

dialog.SetThreadStateChangeCallstackMaxCopyRawStackSize(static_cast<uint16_t>(
settings
.value(kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
orbit_qt::CaptureOptionsDialog::kThreadStateChangeMaxCopyRawStackSizeDefaultValue)
.toUInt()));

int result = dialog.exec();
if (result != QDialog::Accepted) {
Expand Down Expand Up @@ -1387,15 +1377,14 @@ void OrbitMainWindow::on_actionCaptureOptions_triggered() {
dialog.GetLimitLocalMarkerDepthPerCommandBuffer());
settings.setValue(kMaxLocalMarkerDepthPerCommandBufferSettingsKey,
QString::number(dialog.GetMaxLocalMarkerDepthPerCommandBuffer()));
if (absl::GetFlag(FLAGS_tracepoint_callstack_collection)) {
settings.setValue(
kEnableCallStackCollectionOnThreadStateChanges,
static_cast<int>(dialog.GetEnableCallStackCollectionOnThreadStateChanges()
? CaptureOptions::kThreadStateChangeCallStackCollection
: CaptureOptions::kNoThreadStateChangeCallStackCollection));
settings.setValue(kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
static_cast<int>(dialog.GetThreadStateChangeCallstackMaxCopyRawStackSize()));
}
settings.setValue(
kEnableCallStackCollectionOnThreadStateChanges,
static_cast<int>(dialog.GetEnableCallStackCollectionOnThreadStateChanges()
? CaptureOptions::kThreadStateChangeCallStackCollection
: CaptureOptions::kNoThreadStateChangeCallStackCollection));
settings.setValue(kThreadStateChangeCallstackMaxCopyRawStackSizeSettingKey,
static_cast<int>(dialog.GetThreadStateChangeCallstackMaxCopyRawStackSize()));

LoadCaptureOptionsIntoApp();
}

Expand Down