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.

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

[2.x] Bring media assets into the Hyde kernel #1917

Merged

Conversation

caendesilva
Copy link
Member

@caendesilva caendesilva commented Jul 27, 2024

This is part of #1904

See if we can cache the media assets during discovery. A good location may be in the kernel Filesystem instance. This would also provide a small performance optimization if we create cache busting keys at discovery time. It would also improve DX with almost no performance penalty as we could throw an exception when trying to get a non-existent file.

Quick benchmarking shows this now goes from ~2.5ms per MediaFile::files() call to ~2.7ms for the first call, and essentially zero at all subsequent calls in the request lifecycle. This is at the cost of the kernel needing to rerun discovery if media files change during the request, which should only happen in unit testing that are coupled to the filesystem.

@caendesilva caendesilva marked this pull request as draft July 27, 2024 16:36
@caendesilva caendesilva mentioned this pull request Jul 27, 2024
21 tasks
@caendesilva caendesilva force-pushed the bring-media-assets-into-the-hyde-kernel branch from 8a622da to 935e0f9 Compare July 28, 2024 10:46
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (314c57a) to head (94e3f21).

Additional details and impacted files
@@                     Coverage Diff                     @@
##             normalize-the-asset-api     #1917   +/-   ##
===========================================================
  Coverage                     100.00%   100.00%           
- Complexity                      1873      1875    +2     
===========================================================
  Files                            192       193    +1     
  Lines                           4944      4948    +4     
===========================================================
+ Hits                            4944      4948    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@caendesilva
Copy link
Member Author

caendesilva commented Jul 28, 2024

Okay, added a more robust benchmarker in f6b8382, with about 15 files, including nested ones, and a 10MB one.

# Before:
Warmup: 15.8036ms
Benchmark: 10.2105987ms # avg/1000/its

# After:
Warmup: 16.9699ms
Benchmark: 0.00093500000000001ms # avg/1000/its

@caendesilva caendesilva force-pushed the bring-media-assets-into-the-hyde-kernel branch from 935e0f9 to 775a496 Compare July 28, 2024 11:08
@caendesilva caendesilva force-pushed the bring-media-assets-into-the-hyde-kernel branch from 04e94ed to 1d12225 Compare July 28, 2024 11:09
Better matches other file collections
@caendesilva caendesilva force-pushed the bring-media-assets-into-the-hyde-kernel branch from 5389932 to b0f3a89 Compare July 28, 2024 11:20
@caendesilva caendesilva marked this pull request as ready for review July 28, 2024 12:41
@caendesilva caendesilva merged commit e00cbe0 into normalize-the-asset-api Jul 28, 2024
8 checks passed
@caendesilva caendesilva deleted the bring-media-assets-into-the-hyde-kernel branch July 28, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant