Skip to content

CmdPal: Separate in-flight icon loads from cached results - #49738

Merged
Michael Jolley (michaeljolley) merged 1 commit into
mainfrom
dev/jpolasek/f/cmdpal-icon-loading-single-flight
Aug 7, 2026
Merged

CmdPal: Separate in-flight icon loads from cached results#49738
Michael Jolley (michaeljolley) merged 1 commit into
mainfrom
dev/jpolasek/f/cmdpal-icon-loading-single-flight

Conversation

@jiripolasek

@jiripolasek Jiří Polášek (jiripolasek) commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary of the Pull Request

This PR is stacked on #49737 and separates in-flight icon loading from completed-result caching. The XAML request path no longer takes a provider-wide lock, while concurrent requests for the same icon still share a single task and completion cleanup can retire only that exact task generation.

The loader now reports whether queue admission succeeded, preventing requests rejected during shutdown from remaining pending indefinitely.

Overall, it seems that we got rid of a lock and actually reduced turnaround on UI thread. The separate in-flight tracking takes its tall, but overall throughput increased (tl;dr - it's faster).

  • Deduplicate outstanding icon loads in a dedicated in-flight dictionary.
  • Cache successful tasks before atomically retiring their in-flight entries.
  • Keep failed and rejected loads out of the adaptive cache so they can be retried.
  • Replace EnqueueLoad with TryEnqueueLoad and fault rejected requests.
  • Add tests for concurrent loading, publication ordering, failures, rejection, and retries.

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end-user-facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@zadjii-msft

Copy link
Copy Markdown
Member

This might low key fix #47620. I'm trying to stand up a feature regression test in run to actually confirm that...

@zadjii-msft Mike Griese (zadjii-msft) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense to me, and looks like it's way more async, which is what we want.

I'm just weirded out about these shims in the cmdpal.ui.unitests, but whatever that's not worth blocking on.


namespace Microsoft.CmdPal.UI.ViewModels;

internal sealed class IconDataViewModel

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are... fake view models? but in the Microsoft.CmdPal.UI.ViewModels namespace?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh squirrel, I'm pretty sure I hid them inside the test class (bandit), puzzling.

As for why: it builds much faster. The exe is self-contained, so the test project has to be too. I gave up on solving that problem in this PR, leaving us with a great conversation starter :)

Base automatically changed from dev/jpolasek/f/cmdpal-adaptive-cache-reentrancy to main August 7, 2026 17:40
- Remove the provider-wide lock from the XAML icon request path.
- Deduplicate concurrent loads with an identity-safe in-flight dictionary.
- Cache successful results before atomically retiring their in-flight tasks.
- Report queue rejection through TryEnqueueLoad and fault rejected requests.
- Add tests for concurrent loading, caching, failures, and retries.
@zadjii-msft
Mike Griese (zadjii-msft) force-pushed the dev/jpolasek/f/cmdpal-icon-loading-single-flight branch from 2b6739d to 9981c06 Compare August 7, 2026 17:40
@michaeljolley
Michael Jolley (michaeljolley) merged commit 56025d7 into main Aug 7, 2026
8 checks passed
@michaeljolley
Michael Jolley (michaeljolley) deleted the dev/jpolasek/f/cmdpal-icon-loading-single-flight branch August 7, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.101 Product-Command Palette Refers to the Command Palette utility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants