[Impeller][GLES] Link programs in parallel on ANGLE with GL_KHR_parallel_shader_compile - #193066

The global pointer deferred_links is set to point to the local variable state. When RunDeferredLink exits, state is destroyed, leaving deferred_links as a dangling pointer. This can cause undefined behavior or crashes if subsequent tests or teardown procedures invoke mock GL functions that dereference it. Consider using a local RAII helper to automatically reset deferred_links to nullptr upon exiting the function.

  deferred_links = &state;
  struct ScopedReset {
    ~ScopedReset() { deferred_links = nullptr; }
  } reset;

Fixed with local variant

  fml::ScopedCleanupClosure reset_deferred_links(
      []() { deferred_links = nullptr; });

I think this batched finishing can cause the following situation:

1. Pipeline A is popped from pending_jobs_ in pipeline_compile_queue.cc, starts linking, and is put into pending_pipelines_

2. Pipeline B is popped from pending_jobs_, starts linking, and is put into pending_pipelines_

3. Pipeline A is requested for immediate compilation and use (e.g., Pipeline::WaitAndGet -> PipelineCompileQueue::PerformJobEagerly)

In this case, WaitAndGet will call PerformJobEagerly. Before, this would result in the pipeline being scheduled for immediate compilation. With these changes, PerformJobEagerly will return as a no-op, since Pipeline A isn't found in pending_jobs_. The upstream caller of WaitAndGet will block until pending_count >= library.max_pending_links and everything in pending_pipelines_ is finished (e.g., the compilation of a few more pipelines).

I can think of two ways to resolve this - there are maybe better ways :)

1. Wire up PipelineCompileQueue::PerformJobEagerly to be able to finish pipelines from pending_pipelines_ directly. This seems kind of ugly, but maybe there's a nice way to do it.

2. GL_KHR_parallel_shader_compile comes with GL_COMPLETION_STATUS_KHR, a non-blocking version of GL_LINK_STATUS. We can use that to scan through pending_pipelines_ each time this function is called, and immediately call FinishProgram on them if GL_COMPLETION_STATUS_KHR == TRUE.

2 seems nice because faster pipelines and pipelines that have already completed compilation don't have to wait for pending_count >= library.max_pending_links, it may speed things up overall.

thoughts on this?

Once link is started, the pipeline is no longer in pending_jobs_, so PerformJobEagerly can't help and WaitAndGet waits for the whole batch.

I went with option 2, thanks for this good finding, but it's overall win ended in very random ~-1-10% of startup time, which is 4% faster on average on real mini pc. Before each compile job adds its pipeline, it asks GL_COMPLETION_STATUS_KHR for the pending links and finishes the ones that completed. If no job runs afterwards, the drain check finishes them as before. If a driver rejects the query, the value stays GL_FALSE, and those links are checked at the limit or on drain like before. Tests: FinishesCompletedLinkBeforeLimit, KeepsLinkPendingWhenCompletionQueryFails.

Tested this again on both mini pc and vm:
Same two profile apps, flutter_gallery, timed from process start to its first visible window, and app with 8 FragmentProgram shaders, timed from process start to its first rasterized frame. Every comparison uses one engine binary with environment switches, 9 to 10 rounds per variant in rotating order.

The case you mentioned happens rarely. The raster thread waited on a started link in 2 of 8 gallery runs on the mini PC, 33 to 34 ms each, and in 1 of 5 on the VM, 17 ms. I never saw it in the 8 shader app no matter how many times i launched it (in more than 100 launches).

Results, median first frame o 8 shader app, ms:

On mini PC first frame comes 1-10% earlier, depending on the session, and never later. On VM the difference was too small to tell it apart from noise. The gallery start time did not change on either machine. After the first frame both versions run at about 120 FPS.

Also retested (again) closing the window while programs are still linking: 60 runs per machine, some of them with links slowed down on purpose. Every run exited normally with code 0 and no errors in the lo.

One more finding that I did not include. My first draft of this change was 3 to 8% faster than the final version. The reason was in debug log line in every job. Regardless of how ridiculous it sounds, writing it took about 0.4 ms, and that short pause of IO thread made the startup faster. Adding pause on purpose gives the same effect.

My guess why: without a pause, the IO thread checks again before the driver has finished any link. The pending list then reaches the limit, and the IO thread has to wait for the slowest link. With that pause, more links are already done when it checks. With the sleep, on mini PC the time spent waiting at the limit dropped down from 906 ms to 560 ms.

The pause looks awkwardly hacky, so I didn't include it, but I can add it if you want.

My guess is just a guess so I don't fully understand why it helps, and its effect depends on OS: on Windows sleep_for(100us) actually sleeps 1 to 2 ms, and yield() helped more on the VM than on my mini PC. It also makes the first frame faster to appear, while some links are still running, so right after the first frame there is one longer gap: 139 ms instead of 91 on the mini PC. The same gap appears without the pause whenever the first frame happens to come too early. In total more frames are shown by 3 s with the pause, 162 instead of 138.

Mini PC and VM are the same mentioned in description Ryzen AI 9 HX 370 with Radeon 890M, and Windows VM on Parallels on Macbook M4 Max

Nice! I'm glad that this sped things up.

@gaaclarke, Thoughts on the behavior change in WaitAndGet -> PerformJobEagerly? IMO with the addition of the continuous polling, the behavior change is negligible.

The initialization of weak_compile_queue can be simplified and made more readable by default-constructing the std::weak_ptr and conditionally assigning it, rather than using a ternary operator with an explicit temporary std::shared_ptr construction. This avoids unnecessary temporary object creation and improves code clarity.

  std::weak_ptr<PipelineCompileQueueGLES> weak_compile_queue;
  if (async && supports_parallel_shader_compile_) {
    weak_compile_queue = compile_queue_;
  }

Didn't know the ternary makes a temporary shared_ptr copy 3 atomic ops instead of 1 with if / else

It's because your last term is std::shared_ptr<PipelineCompileQueueGLES>(), not std::weak_ptr<PipelineCompileQueueGLES>(). Otherwise they are the same: https://godbolt.org/z/967Wo81nc

Switch this to return absl::StatusOr please.

done

Can you switch the comment to standard docstring format. Spelling out what "deferred" means would be helpful.

done

A comment about where these are deleted is worth mentioning.

done

nit: return absl::Status

done

nit: return absl::status

done

nit: based on what we learned previously from godbolt

done

No, I was thinking that we have the information we need, it just isn't getting plumbed down to the callback and we are recreating that information with the thread_local storage.

I think if we change the callback to PostJobForDescriptor to get called with a bool eager argument we'll have the same information.

using Job = std::function<void(bool eager)>;
bool PostJobForDescriptor(const PipelineDescriptor& desc, const Job& job);

Nit: Can you remove the specific reference to "the change" here. Comments should just reflect the current state of the codebase

Done, removed, just wanted to make sure it's noticed since angle is not just Windows related

Nit: drop the word "both"

Rephrased "both" into what it means in 2 places

I think the logic here and in CreatePipeline is difficult to follow because of the dependencies between variables and the particular logic of std::weak_ptr that makes this work.

IIUC, (!eager && compile_queue) checks that we're (1) not eagerly compiling and

(2a): - We have the lock on the compile_queue_

(2b): - We support_parallel_shader_compile

2a is immediately derivable from the surrounding code, but 2b only works because we've set weak_compile_queue ~30 lines before this (because async && supports_parallel_shader_compile_ == true) and because calling .lock() on an empty std::weak_ptr returns nullptr.

Then, in CreatePipeline, the logic that actually does the async linking is only active if deferred_promise != nullptr - which in turn depends on the complicated chain above.

I recommend breaking the logic here up a bit to be more clear to reason about what logic is being executed when we support parallel shader compile vs not. I'd also recommend adding a check and comment inside of CreatePipeline that ensures deferred_promise == true only if we support parallel shader compile.

Actually, do we still need compile_queue now that we have eager if not, the check inside the job lambda could just be:

const bool defer_link =                                                                                    
 !eager && library.SupportsParallelShaderCompile();                                                     
if (defer_link) {                                                                                          
// The promise is resolved later in FinishCompletedPipelines or                                          
// FinishPendingPipelines unless starting the link fails right away.                                     
    if (!CreatePipeline(weak_this, descriptor, vert_function, frag_function,                                 
        threadsafe, promise)) {                                                              
    promise->set_value(nullptr);                                                                           
    }                                                                                                        
    return;                                                                                                  
}

That + the comments and checks in CreatePipeline would make this a lot clearer

Actually, do we still need compile_queue now that we have eager if not, the check inside the job lambda could just be:

const bool defer_link =                                                                                    
 !eager && library.SupportsParallelShaderCompile();                                                     
if (defer_link) {                                                                                          
// The promise is resolved later in FinishCompletedPipelines or                                          
// FinishPendingPipelines unless starting the link fails right away.                                     
    if (!CreatePipeline(weak_this, descriptor, vert_function, frag_function,                                 
        threadsafe, promise)) {                                                              
    promise->set_value(nullptr);                                                                           
    }                                                                                                        
    return;                                                                                                  
}

That + the comments and checks in CreatePipeline would make this a lot clearer

The lock only fails once the queue is gone, which shouldn't happen while the library lives, but we'd rather keep it as a guard than rely on that, i'll add a commit soon with requested changes + extra test

Since design doc was requested, I kept this small so we don't change the same code twice, the extension check is now an explicit may_defer_link in the condition, CreatePipeline DCHECK it with comment, and AsyncJobWithoutExtensionLinksSynchronously covers this scenario

The confusing part was left over from the eager change, the weak_ptr lock used to tell whether we were inside a queue job