Skip to content

Conversation

@eth3lbert
Copy link
Contributor

@eth3lbert eth3lbert commented Nov 25, 2024

This PR migrate render_pkg_readme() and find_file_by_path() fns to async/await.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

is the text_to_html() call fast enough to be performed in an async worker thread? I'm wondering if we should keep it in a spawn_blocking() call

@eth3lbert
Copy link
Contributor Author

is the text_to_html() call fast enough to be performed in an async worker thread? I'm wondering if we should keep it in a spawn_blocking() call

Good call. I'll answer this question after I experiment with it after I get up :)

@Turbo87
Copy link
Member

Turbo87 commented Nov 25, 2024

I also kept it in spawn_blocking() in the corresponding background job for now:

let rendered = spawn_blocking(move || {
text_to_html(
&job.text,
&job.readme_path,
job.base_url.as_deref(),
job.pkg_path_in_vcs.as_ref(),
)
})
.await?;

@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Nov 25, 2024
@eth3lbert eth3lbert force-pushed the async-render-readmes branch from 0b5a4d2 to 82db663 Compare November 25, 2024 16:15
@eth3lbert
Copy link
Contributor Author

After benchmarking, I believe we need to keep spawn_blocking(). Most markdown files should not take longer than 6ms, but there might be some files that aren't large but still take a lot of time(maybe seconds), which could cause blocking.

@eth3lbert eth3lbert force-pushed the async-render-readmes branch from 82db663 to f063b44 Compare November 26, 2024 08:38
@eth3lbert eth3lbert changed the title admin/render_readmes: Remove spawn_blocking() call admin/render_readmes: Migrate render_pkg_readme() and find_file_by_path() fns to async/await Nov 26, 2024
@eth3lbert eth3lbert force-pushed the async-render-readmes branch from f063b44 to 4e9cd5e Compare November 26, 2024 08:42
@codecov
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 78.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.47%. Comparing base (b294cee) to head (372f5c0).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/bin/crates-admin/render_readmes.rs 78.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10070      +/-   ##
==========================================
- Coverage   89.48%   89.47%   -0.01%     
==========================================
  Files         295      295              
  Lines       31260    31271      +11     
==========================================
+ Hits        27973    27980       +7     
- Misses       3287     3291       +4     

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

…_path()` fns to async/await

It would be great if we could just remove the `spawn_blocking()` call,
but the `text_to_html()` call could be blocking in some cases.
@eth3lbert eth3lbert force-pushed the async-render-readmes branch from 4e9cd5e to 372f5c0 Compare November 27, 2024 00:12
@eth3lbert
Copy link
Contributor Author

Ah, it seems I didn't commit the modified part correctly for some reason.
I've rebased to the latest main and made slight modifications to align with our recent work.

@Turbo87 Turbo87 merged commit 1a0a3d0 into rust-lang:main Nov 27, 2024
10 checks passed
@eth3lbert eth3lbert deleted the async-render-readmes branch November 27, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants