这是indexloc提供的服务，不要输入任何密码
Skip to content

perf: MRD improvements #3525

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

perf: MRD improvements #3525

wants to merge 7 commits into from
+106 −93

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Jul 15, 2025 •
edited
Loading

Description

This PR optimizes CPU runtime by removing unused and unnecessary code paths within the Multi Range Downloader (MRD) flow.

During CPU profiling of the random reads flow (50GB file, 4KB I/O), we identified three key areas consuming significant CPU time that could be optimized:

  1. Duplicate Tracing: Redundant tracing logging within the MRD flow.
  2. Excessive UUID Generation: Frequent UUID.New() calls for log IDs.
  3. Unnecessary Interrupt Select Statement: A select statement for interrupt handling that was always active, even when not needed.

This PR addresses these issues with the following changes:

  1. Streamlined Tracing: Trace logs have been removed from the MRD wrapper. The debug bucket is now only included as a wrapper when TRACE logs are explicitly enabled, preventing unnecessary overhead.
  2. Eliminated UUID Generation: UUID.New() calls are no longer performed in the MRD wrapper, as logging is no longer conducted at this level.
  3. Conditional Interrupt Handling: The select statement for interrupt handling is now conditionally enabled, activating only when a user explicitly configures interrupt handling.

NOTE: bug b/432639555 was identified during the implementation. This will be fixed in subsequent PR.

Link to the issue in case of a bug fix.

b/423600392

Testing details

  1. Manual - NA
  2. Unit tests - updated
  3. Integration tests - NA
  4. Profiling/Perf:
    Profiling:
    Before:
87V2StUjyGV9WZ8

After:
8U8hWZavY9KSVNH

Bandwidth:
50G 4K IO:
Before changes: 3976kB/s
After changes: 4343kB/s (~10% improvement)

Any backward incompatible change? If so, please explain.

@ashmeenkaur ashmeenkaur added execute-integration-tests Run only integration tests execute-integration-tests-on-zb To run E2E tests on zonal bucket. labels Jul 15, 2025
@ashmeenkaur ashmeenkaur changed the title Perf: MRD improvements perf: MRD improvements Jul 15, 2025
@ashmeenkaur ashmeenkaur removed execute-integration-tests Run only integration tests execute-integration-tests-on-zb To run E2E tests on zonal bucket. labels Jul 16, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 18, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.02041% with 24 lines in your changes missing coverage. Please review.

Project coverage is 79.47%. Comparing base (8d6d355) to head (fb02955).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
internal/storage/debug_bucket.go 0.00% 18 Missing ⚠️
internal/gcsx/bucket_manager.go 0.00% 3 Missing and 1 partial ⚠️
cmd/mount.go 0.00% 1 Missing ⚠️
internal/storage/bucket_handle.go 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3525      +/-   ##
==========================================
- Coverage   79.70%   79.47%   -0.23%     
==========================================
  Files         141      141              
  Lines       18931    18939       +8     
==========================================
- Hits        15089    15052      -37     
- Misses       3330     3375      +45     
  Partials      512      512
Flag Coverage Δ
unittests 79.47% <51.02%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ashmeenkaur ashmeenkaur force-pushed the mrd-imp branch 2 times, most recently from 72eeef1 to 98d8c5d Compare July 18, 2025 09:49
@ashmeenkaur ashmeenkaur added the execute-integration-tests Run only integration tests label Jul 21, 2025
@ashmeenkaur ashmeenkaur marked this pull request as ready for review July 21, 2025 10:45
@ashmeenkaur ashmeenkaur requested a review from a team as a code owner July 21, 2025 10:45
@ashmeenkaur ashmeenkaur requested a review from charith87 July 21, 2025 10:45
@github-actions github-actions bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Jul 21, 2025
@kislaykishore kislaykishore requested review from a team and Tulsishah and removed request for vadlakondaswetha and a team July 21, 2025 10:46
@ashmeenkaur ashmeenkaur requested review from vadlakondaswetha and removed request for charith87 July 21, 2025 10:46
vadlakondaswetha
vadlakondaswetha reviewed Jul 22, 2025
View reviewed changes
internal/gcsx/multi_range_downloader_wrapper.go
case res := <-done:
if !mrdWrapper.config.FileSystem.IgnoreInterrupts {
select {
case <-time.After(timeout):
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove the timeout since its currently set to a very higher value @abhishek10004

@github-actions GitHub Actions
Copy link

Hi @Tulsishah, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadlakondaswetha vadlakondaswetha vadlakondaswetha left review comments

@Tulsishah Tulsishah Awaiting requested review from Tulsishah Tulsishah was automatically assigned from GoogleCloudPlatform/gcsfuse-shadow-reviewers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
execute-integration-tests Run only integration tests remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ashmeenkaur @vadlakondaswetha