[fuchsia][shader warmup] fix for fxbug.dev/90387

[freiling]

Warmup surface was being initialized on the platform thread rather than
the raster thread which was causing races in the skia cache, this change
just moves the warmup surface intiialization to the raster thread.

This change also addresses another minor problem where invoking shader
warmup with 0 skps would drop the callback on the floor and that would
cause the dart future to hang forever. Simply invoke the dart future if
no SKP's are provided.

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[arbreng]

+2 so you can land it, but please fix the static thing first

[arbreng]

I was confused about the static inside a lambda here, so I looked it up: https://stackoverflow.com/questions/8391058/how-do-static-variables-in-lambda-function-objects-work/35038341 and https://stackoverflow.com/questions/55510/when-do-function-level-static-variables-get-allocated-initialized/58804#58804

This will work for the first time shader warmup is done within a given process, but subsequent warmup requests will fail mysteriously with the "Failed to create" error. This is because the static will be re-used across all lambda invocations....

Since you delete skp_warmup_surface when you're done with it below, it will be nullptr forever after and subsequent shader warmups will always fail (and it'll be really hard to debug).

It seems like you'll have to post 1 task to create the surface, and then post a bunch more inside the for loop which use the surface as a capture.

[freiling]

oh damn good catch I didnt even think about that with the lambda, lemme chew on that and I'll cook up a solution. I guess i can move the pointer declaration to the outer function and then just initialize the pointer in the lambda. That way well get a new pointer for each invocation of WarmupSkps

[freiling]

Yeah will do ASAP. Heater in my house is broken so dealing with that first while theres daylight, will try to fix the static thing and land by EOD

[freiling]

I think this is right but I need to get my arm workflow working to test on arm before i land, pls dont land this without talking to me

[akbiggs]

Sorry for the slow review.

[akbiggs]

if (*skp_warmup_surface == nullptr) might be more readable here and below.

[freiling]

done

[akbiggs]

Optional: We could simplify invoking completion_callback here a bit by doing:

void Engine::WarmupSkps(..., std::optional<std::function<void(uint32_t)>> maybe_completion_callback) {
  auto completion_callback = [maybe_completion_callback](uint32_t skp_count) {
    if (maybe_completion_callback.has_value() && maybe_completion_callback.value()) {
      maybe_completion_callback.value()(skp_count);
    }
  };
  ...
}

That way we can invoke it like:

completion_callback(0);

But it might obfuscate things a bit too much so leaving this as optional.

[freiling]

good idea I like it :) done.

[akbiggs]

Is it okay that we're doing delete static_cast<SurfaceProducerSurface*>(skp_warmup_surface); on this below as though it were a SurfaceProducerSurface* instead of a unique_ptr<SurfaceProducerSurface*>?

[akbiggs]

NVM, I see that skp_warmup_surface is redefined within that scope.

[freiling]

no you're right thats very bad and will cause crashes. Good catch. Looks like the tests werent catching this because they tear down the context before it completes so the callback never runs. I'll fix this and plumb the tests to wait the for completion properly so theyll catch things like this.