Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] fail CI if validations are enabled but not available. #51378

Merged
auto-submit merged 6 commits into from
Apr 5, 2024
Merged

[Impeller] fail CI if validations are enabled but not available. #51378

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82b4fad
[Impeller] fail CI if validations are enabled but not available.
Mar 13, 2024
cb489e4
Oh no I broke runtests.py with a refactor.
Mar 13, 2024
eee1102
++
Mar 14, 2024
85f1e7c
Merge branch 'main' into fail_if_no_validation
Mar 14, 2024
c588660
Merge branch 'main' into fail_if_no_validation
Mar 19, 2024
1309a27
Merge branch 'main' into fail_if_no_validation
Apr 5, 2024
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: 3 additions & 0 deletions impeller/playground/backend/vulkan/playground_impl_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)
context_settings.shader_libraries_data = ShaderLibraryMappingsForPlayground();
context_settings.cache_directory = fml::paths::GetCachesDirectory();
context_settings.enable_validation = switches_.enable_vulkan_validation;
context_settings.fatal_missing_validations =
Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

Where is this getting set to true for the test runner?

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

IIRC both goldens and regular playground tests go through this path to create the context.

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

I'm not sure. I'd set a breakpoint in the impeller_golden_tests here to make sure. We swap out playground tests for golden tests in that target so this might not be getting hit. That's why we should make sure we have the test for that executable too (as mentioned in the other comment).

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

Both impeller unittests and goldens have failed on the commit where I broke run_tests.py:

https://github.com/flutter/engine/pull/51378/checks?check_run_id=22626102272

I think that means its working.

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

The golden test runner failed from a flake:

[  FAILED  ] FlutterEngineTest.CanOverrideBackgroundColor

 1 FAILED TEST

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

No its in there: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753556488722177297/+/u/test:_Impeller-golden__dart_and_engine_tests_for_host_release/stdout

It did hit a flake during one of the reruns though.

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

That failure is when executing impeller_unittests. Let's GVC when you have a chance. I think I can clear this up in under 5 minutes over gvc =)

switches_.enable_vulkan_validation;
;

auto context_vk = ContextVK::Create(std::move(context_settings));
if (!context_vk || !context_vk->IsValid()) {
Expand Down
6 changes: 5 additions & 1 deletion impeller/renderer/backend/vulkan/capabilities_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ namespace impeller {

static constexpr const char* kInstanceLayer = "ImpellerInstance";

CapabilitiesVK::CapabilitiesVK(bool enable_validations) {
CapabilitiesVK::CapabilitiesVK(bool enable_validations,
bool fatal_missing_validations) {
auto extensions = vk::enumerateInstanceExtensionProperties();
auto layers = vk::enumerateInstanceLayerProperties();

Expand Down Expand Up @@ -45,6 +46,9 @@ CapabilitiesVK::CapabilitiesVK(bool enable_validations) {
<< "Requested Impeller context creation with validations but the "
"validation layers could not be found. Expect no Vulkan validation "
"checks!";
if (fatal_missing_validations) {
FML_LOG(FATAL) << "Validation missing. Exiting.";
}
}
if (validations_enabled_) {
FML_LOG(INFO) << "Vulkan validations are enabled.";
Expand Down
3 changes: 2 additions & 1 deletion impeller/renderer/backend/vulkan/capabilities_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ enum class OptionalDeviceExtensionVK : uint32_t {
class CapabilitiesVK final : public Capabilities,
public BackendCast<CapabilitiesVK, Capabilities> {
public:
explicit CapabilitiesVK(bool enable_validations);
explicit CapabilitiesVK(bool enable_validations,
bool fatal_missing_validations = false);

~CapabilitiesVK();

Expand Down
4 changes: 2 additions & 2 deletions impeller/renderer/backend/vulkan/context_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,8 @@ void ContextVK::Setup(Settings settings) {
enable_validation = true;
#endif

auto caps =
std::shared_ptr<CapabilitiesVK>(new CapabilitiesVK(enable_validation));
auto caps = std::shared_ptr<CapabilitiesVK>(new CapabilitiesVK(
enable_validation, settings.fatal_missing_validations));

if (!caps->IsValid()) {
VALIDATION_LOG << "Could not determine device capabilities.";
Expand Down
2 changes: 2 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ class ContextVK final : public Context,
fml::UniqueFD cache_directory;
bool enable_validation = false;
bool enable_gpu_tracing = false;
/// If validations are requested but cannot be enabled, log a fatal error.
bool fatal_missing_validations = false;

Settings() = default;

Expand Down
11 changes: 11 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,5 +222,16 @@ TEST(ContextVKTest, WarmUpFunctionCreatesRenderPass) {
"vkCreateRenderPass") != functions->end());
}

TEST(ContextVKTest, FatalMissingValidations) {
Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

Is this compiled into impeller_golden_tests? We should have one of these for that target too.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

This is just a check that the setting works. I've enabled this setting for playgrounds, so I should be able to confirm that removing any of the VVL config causes failures.

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

What I meant is that we want to make sure that impeller_golden_tests --gtest_filter=ContextVKTest.FatalMissingValidations is executed too. I suspect this will execute for impeller_tests but not impeller_golden_tests.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

That doesn't matter though, that isn't what is running the check.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 13, 2024

Choose a reason for hiding this comment

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

its just a test that the check works

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 13, 2024

Choose a reason for hiding this comment

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

its just a test that the check works

I know =)

that isn't what is running the check.

I know, I want this test running in both executables. In impeller_unittests and impeller_golden_tests we should fail if we don't have vulkan validation.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 14, 2024

Choose a reason for hiding this comment

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

I looked into this more. It doesn't seem possible for me to set up a playground where validations are requested but still misisng, without mocking out the loader. Did you have a different idea here?

EXPECT_DEATH(const std::shared_ptr<ContextVK> context =
Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 19, 2024

Choose a reason for hiding this comment

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

Are you getting a warning about running this in a multithreaded environment? This works by forking and catching signals which won't work reliably on multi threaded processes.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 19, 2024

Choose a reason for hiding this comment

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

Not running locally

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 19, 2024

Choose a reason for hiding this comment

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

Is there a cmd line flag I can test this locally with?

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 19, 2024

Choose a reason for hiding this comment

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

The tests that you execute before your test are going to matter. If ever there is any test that can spawn a thread and not join it, like creating a thread pool, that run before this test it will be a problem. I'd just try running the test runner with every test enabled. Maybe try enabling the test shuffle a few times and seeing if it prints out the error. I almost guarantee there is at least one test that is leaking threads though. That's what I was seeing with the golden test runner at least.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 19, 2024

Choose a reason for hiding this comment

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

gtest parallel runs multiple gtest processes, if one or more of those process spawn a subprocess that shouldn't cause any issues. The problems we've had usually stem from reading/writing to the file system.

Copy link
Copy Markdown
Member

@gaaclarke gaaclarke Mar 19, 2024

Choose a reason for hiding this comment

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

The problem isn't from running the tests in multiple processes, it comes from the signal function. Which thread will handle a signal is undefined. That's what's being used to catch the death of the process. I'm pretty sure that's why googletest prints out the warning. If you don't see the warning and want to give it a shot that sounds good to me. We should add a comment to the test in case it ever starts flaking.

MockVulkanContextBuilder()
.SetSettingsCallback([](ContextVK::Settings& settings) {
settings.enable_validation = true;
settings.fatal_missing_validations = true;
})
.Build(),
"");
}

} // namespace testing
} // namespace impeller