Skip to content

use max size semaphore #4220

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
Merged
GarrettBeatty merged 1 commit into from
Dec 11, 2025
Merged

use max size semaphore #4220

GarrettBeatty merged 1 commit into from
Dec 11, 2025

Conversation

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty commented Dec 10, 2025 •
edited
Loading

Stacked PRs:

Description

I noticed during testing using AI that it said the maxinmemoryparts being used from the logs was 1026 even though i had configured it to 1024. this means i was:

  1. Releasing the semaphore twice
  2. Not enforcing a maximum amount for the sempahore

This change updates the code to fix the double release and also ensure the semaphores enforce a maximum.

Motivation and Context

#3806

Testing

  1. Reran all unit tests and they pass
  2. Reran performance tests and they have same performance
  3. dry run in progress 16f30d8d-ec62-40bb-ab69-5f50383ed393 pass

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/2 branch from aeec2e8 to ef08cbf Compare December 10, 2025 15:52
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/3 branch from 0b21da5 to 9062721 Compare December 10, 2025 15:52
This was referenced Dec 10, 2025
Merged
Merged
Closed
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/gcbeatty/taskoptimization/2 to feature/transfermanager December 10, 2025 18:49
GarrettBeatty added a commit that referenced this pull request Dec 10, 2025
@GarrettBeatty
use max size semaphore
a0cbc80 
stack-info: PR: #4220, branch: GarrettBeatty/gcbeatty/taskoptimization/3
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/3 branch from 9062721 to a0cbc80 Compare December 10, 2025 18:49
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/gcbeatty/taskoptimization/2 December 10, 2025 18:50
GarrettBeatty
GarrettBeatty commented Dec 10, 2025
View reviewed changes
sdk/test/Services/S3/UnitTests/Custom/BufferedPartDataHandlerTests.cs
Assert.AreEqual(3, bufferedPartNumbers[0], "Part 3 should be buffered");

// Verify ReleaseBufferSpace was called for streaming path (immediate capacity release)
mockBufferManager.Verify(m => m.ReleaseBufferSpace(), Times.Once,
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Dec 10, 2025

Choose a reason for hiding this comment

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

this isnt true anymore since we hold it until the reader consumes it which is why i removed it

GarrettBeatty
GarrettBeatty commented Dec 10, 2025
View reviewed changes
sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs
streamingDataSource = null;

// Release capacity immediately since we're not holding anything in memory
_partBufferManager.ReleaseBufferSpace();
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this was being released here and then also when the reader read the part later on. so we remove this to fix the double release.

Side note:
The original reason i had this hear was because my intent was not count streaming to the user as "holding the part in memory". But for simplicitys sake and this bug fix, we now consider a part that gets directly streamed to the user as "taking up memory". in the future we could probably optimize/change this if we want.

GarrettBeatty
GarrettBeatty commented Dec 10, 2025
View reviewed changes
sdk/src/Services/S3/Custom/Transfer/Internal/BufferedPartDataHandler.cs
_partBufferManager.ReleaseBufferSpace();

_logger.DebugFormat("BufferedPartDataHandler: [Part {0}] StreamingDataSource added and capacity released",
partNumber);
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Dec 10, 2025

Choose a reason for hiding this comment

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

this was being released here and then also when the reader read the part later on. so we remove this to fix the double release.

Side note:
The original reason i had this hear was because my intent was not count streaming to the user as "holding the part in memory". But for simplicity's takes and this bug fix we now consider a part that gets directly streamed to the user as "taking up memory". in the future we could probably optimize/change this if we want.

GarrettBeatty
GarrettBeatty commented Dec 10, 2025
View reviewed changes
sdk/test/Services/S3/UnitTests/Custom/MultipartDownloadManagerTests.cs
{
// Arrange
var mockClient = MultipartDownloadTestHelpers.CreateMockS3Client();
var mockResponse = MultipartDownloadTestHelpers.CreateSinglePartResponse(1024);
Copy link
Contributor Author

@GarrettBeatty GarrettBeatty Dec 10, 2025

Choose a reason for hiding this comment

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

had to update all of these tests to call DiscoverDownloadStrategyAsync/reservere buffer space before doing any action that would release buffer space. otherwise it was getting exceeded semaphore maximum error (which is expected now)

@GarrettBeatty GarrettBeatty changed the title use max size semaphore Multi part download, make semaphores have max size Dec 10, 2025
@GarrettBeatty GarrettBeatty marked this pull request as ready for review December 10, 2025 19:08
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/gcbeatty/taskoptimization/2 to feature/transfermanager December 10, 2025 22:39
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/3 branch from a0cbc80 to 8b5d911 Compare December 10, 2025 22:39
@GarrettBeatty GarrettBeatty changed the title Multi part download, make semaphores have max size use max size semaphore Dec 10, 2025
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/gcbeatty/taskoptimization/2 December 10, 2025 22:40
@GarrettBeatty GarrettBeatty mentioned this pull request Dec 10, 2025
Open
7 tasks
@GarrettBeatty GarrettBeatty changed the base branch from GarrettBeatty/gcbeatty/taskoptimization/2 to feature/transfermanager December 11, 2025 14:27
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/3 branch from 8b5d911 to 0942990 Compare December 11, 2025 14:27
@GarrettBeatty GarrettBeatty changed the base branch from feature/transfermanager to GarrettBeatty/gcbeatty/taskoptimization/2 December 11, 2025 14:27
Base automatically changed from GarrettBeatty/gcbeatty/taskoptimization/2 to feature/transfermanager December 11, 2025 14:28
@GarrettBeatty
use max size semaphore
58d4444 
stack-info: PR: #4220, branch: GarrettBeatty/gcbeatty/taskoptimization/3
@GarrettBeatty GarrettBeatty force-pushed the GarrettBeatty/gcbeatty/taskoptimization/3 branch from 0942990 to 58d4444 Compare December 11, 2025 14:28
@GarrettBeatty GarrettBeatty merged commit 821729c into feature/transfermanager Dec 11, 2025
1 check passed
@GarrettBeatty GarrettBeatty deleted the GarrettBeatty/gcbeatty/taskoptimization/3 branch December 11, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@normj normj normj approved these changes

@philasmar philasmar philasmar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GarrettBeatty @normj @philasmar