From 27cb96d8dab85aeb9a5871fb5c5ae5862ca41a8d Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 4 Aug 2026 10:30:53 +0300 Subject: [PATCH 1/4] perf(s3): LIST-based existence + parallel tile upload (MAPCO-11322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scoped I/O wins for the S3 path (full end-to-end async deferred to its own change). - S3Client.GetTileKey: existence/key lookup did a full GetObjectAsync, which downloads the whole object body just to read its key. Use a single prefixed ListObjectsV2 (MaxKeys=1) instead — no body transfer. - S3.InternalUpdateTiles: upload the batch's tiles with Parallel.ForEach instead of one blocking PutObject at a time. Tiles are materialized first so the single-threaded grid/origin projection runs before the uploads fan out; the S3 client and PutObject are independent per tile. - Raise ServicePointManager.DefaultConnectionLimit (default 2) so parallel PUTs are not serialized on the connection pool. TileExists client test now drives ListObjectsV2; the S3 UpdateTiles data-type test no longer sequences UpdateTile (uploads are unordered) but still asserts one upload per tile. Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/Clients/S3Client.cs | 24 ++++++------------- MergerLogic/DataTypes/S3.cs | 7 +++--- .../Extensions/ServiceCollectionExtensions.cs | 5 ++++ MergerLogicUnitTests/DataTypes/S3Test.cs | 3 ++- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 23 +++++++----------- 5 files changed, 25 insertions(+), 37 deletions(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index f06c0e23..f6992de5 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -5,6 +5,7 @@ using MergerLogic.ImageProcessing; using MergerLogic.Utils; using Microsoft.Extensions.Logging; +using System.Linq; using System.Reflection; namespace MergerLogic.Clients @@ -154,23 +155,12 @@ public void UpdateTile(Tile tile) string methodName = MethodBase.GetCurrentMethod().Name; string keyPrefix = this._pathUtils.GetTilePathWithoutExtension(this.path, z, x, y, true); - try - { - var getRequest = new GetObjectRequest { BucketName = this._bucket, Key = keyPrefix }; - var getObjectTask = this._client.GetObjectAsync(getRequest); - string result = getObjectTask.Result.Key; - return result; - } - catch (AggregateException e) - { - if (IsKeyError(e)) - { - this._logger.LogDebug($"[{methodName}] error getting key: {e.Message}"); - return null; - } - // In case there are other errors such as connection to S3 - throw e; - } + // A single prefixed LIST (MaxKeys=1) resolves existence and the real extension without + // downloading the object body the way GetObject did. + var listRequest = new ListObjectsV2Request { BucketName = this._bucket, Prefix = keyPrefix, MaxKeys = 1 }; + this._logger.LogDebug($"[{methodName}] ListObjectsV2Async BucketName: {this._bucket}, Prefix: {keyPrefix}"); + var listObjectsTask = this._client.ListObjectsV2Async(listRequest); + return listObjectsTask.Result.S3Objects.FirstOrDefault()?.Key; } } } diff --git a/MergerLogic/DataTypes/S3.cs b/MergerLogic/DataTypes/S3.cs index 84cdd79e..fca3ead1 100644 --- a/MergerLogic/DataTypes/S3.cs +++ b/MergerLogic/DataTypes/S3.cs @@ -198,10 +198,9 @@ public override long TileCount() protected override void InternalUpdateTiles(IEnumerable targetTiles) { this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] start"); - foreach (var tile in targetTiles) - { - this.Utils.UpdateTile(tile); - } + // Materialize first so the upstream (single-threaded) grid/origin projection runs before the + // uploads fan out; the S3 client and its PutObject calls are independent per tile. + Parallel.ForEach(targetTiles.ToList(), tile => this.Utils.UpdateTile(tile)); this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] end"); } } diff --git a/MergerLogic/Extensions/ServiceCollectionExtensions.cs b/MergerLogic/Extensions/ServiceCollectionExtensions.cs index e3eeda97..353f2943 100644 --- a/MergerLogic/Extensions/ServiceCollectionExtensions.cs +++ b/MergerLogic/Extensions/ServiceCollectionExtensions.cs @@ -107,6 +107,11 @@ public static IServiceCollection RegisterS3(this IServiceCollection collection) MaxErrorRetry = retries, }; + // Raise the per-endpoint connection cap so parallel tile PUTs are not serialized on the + // default limit (2). Set once, before the client's HTTP stack is created. + System.Net.ServicePointManager.DefaultConnectionLimit = + Math.Max(System.Net.ServicePointManager.DefaultConnectionLimit, 100); + var credentials = new BasicAWSCredentials(accessKey, secretKey); return new AmazonS3Client(credentials, s3Config); }); diff --git a/MergerLogicUnitTests/DataTypes/S3Test.cs b/MergerLogicUnitTests/DataTypes/S3Test.cs index 50b286bf..20928669 100644 --- a/MergerLogicUnitTests/DataTypes/S3Test.cs +++ b/MergerLogicUnitTests/DataTypes/S3Test.cs @@ -473,8 +473,9 @@ public void UpdateTiles(bool isOneXOne, GridOrigin origin) if (!isOneXOne || tile.Z != 7) { + // Uploads run in parallel after the (ordered) grid/origin projection, so UpdateTile is + // intentionally outside the strict sequence; per-tile Times.Once is verified below. this._s3UtilsMock - .InSequence(seq) .Setup(utils => utils.UpdateTile(It.IsAny())); } } diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index b3e65ae1..757cd2c2 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -330,24 +330,17 @@ public void TileExists(bool exist) .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) .Returns("key"); + var listResponse = new ListObjectsV2Response(); if (exist) { - this._amazonS3ClientMock - .InSequence(seq) - .Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), - It.IsAny())) - .ReturnsAsync(new GetObjectResponse() { Key = "key" }); + listResponse.S3Objects.Add(new S3Object() { Key = "key" }); } - else - { - this._amazonS3ClientMock + this._amazonS3ClientMock .InSequence(seq) - .Setup(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), + .Setup(s3 => s3.ListObjectsV2Async(It.Is(req => + req.BucketName == "bucket" && req.Prefix == "key" && req.MaxKeys == 1), It.IsAny())) - .ThrowsAsync(new AmazonS3Exception("", Amazon.Runtime.ErrorType.Unknown, "NoSuchKey", "", System.Net.HttpStatusCode.NoContent)); - } + .ReturnsAsync(listResponse); var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); @@ -355,8 +348,8 @@ public void TileExists(bool exist) Assert.AreEqual(exist, s3Utils.TileExists(0, 0, 0)); this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true), Times.Once); - this._amazonS3ClientMock.Verify(s3 => s3.GetObjectAsync(It.Is(req => - req.BucketName == "bucket" && req.Key == "key"), It.IsAny()), Times.Once); + this._amazonS3ClientMock.Verify(s3 => s3.ListObjectsV2Async(It.Is(req => + req.BucketName == "bucket" && req.Prefix == "key" && req.MaxKeys == 1), It.IsAny()), Times.Once); this.VerifyAll(); } From 655c4e3862a98cbd202138ae7efc36219e9420cd Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Wed, 5 Aug 2026 11:57:28 +0300 Subject: [PATCH 2/4] fix: remove no-op ServicePointManager connection cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ServicePointManager.DefaultConnectionLimit governs the legacy HttpWebRequest stack; on net6 the AWS SDK's HttpClient/SocketsHttpHandler ignores it, so the line never capped anything. Drop it and its misleading comment — pure dead-code removal, no behavior change. Bounding the S3 upload fan-out (config-driven MaxDegreeOfParallelism + AmazonS3Config.MaxConnectionsPerServer, value from a load test) is deferred to MAPCO-11364 (P15). Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/Extensions/ServiceCollectionExtensions.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/MergerLogic/Extensions/ServiceCollectionExtensions.cs b/MergerLogic/Extensions/ServiceCollectionExtensions.cs index 353f2943..e3eeda97 100644 --- a/MergerLogic/Extensions/ServiceCollectionExtensions.cs +++ b/MergerLogic/Extensions/ServiceCollectionExtensions.cs @@ -107,11 +107,6 @@ public static IServiceCollection RegisterS3(this IServiceCollection collection) MaxErrorRetry = retries, }; - // Raise the per-endpoint connection cap so parallel tile PUTs are not serialized on the - // default limit (2). Set once, before the client's HTTP stack is created. - System.Net.ServicePointManager.DefaultConnectionLimit = - Math.Max(System.Net.ServicePointManager.DefaultConnectionLimit, 100); - var credentials = new BasicAWSCredentials(accessKey, secretKey); return new AmazonS3Client(credentials, s3Config); }); From 33c76f58a5db68dd7285112b560ad0aba55778d8 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 6 Aug 2026 14:13:17 +0300 Subject: [PATCH 3/4] docs: correct InternalUpdateTiles ToList rationale The projection is effectively pure; ToList is for range-partitioning, not thread-safety. State the real reason (avoids the shared-enumerator lock). Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/DataTypes/S3.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MergerLogic/DataTypes/S3.cs b/MergerLogic/DataTypes/S3.cs index fca3ead1..133cbc68 100644 --- a/MergerLogic/DataTypes/S3.cs +++ b/MergerLogic/DataTypes/S3.cs @@ -198,8 +198,8 @@ public override long TileCount() protected override void InternalUpdateTiles(IEnumerable targetTiles) { this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] start"); - // Materialize first so the upstream (single-threaded) grid/origin projection runs before the - // uploads fan out; the S3 client and its PutObject calls are independent per tile. + // ToList so Parallel.ForEach range-partitions instead of running the upstream grid/origin + // projection under the shared-enumerator lock a bare IEnumerable would impose. Parallel.ForEach(targetTiles.ToList(), tile => this.Utils.UpdateTile(tile)); this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] end"); } From c96d7ae1f243882a3f4c088d882ea7435bc9d67a Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 13:44:57 +0300 Subject: [PATCH 4/4] perf(s3): resolve existence via HEAD per extension, not prefix LIST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A prefix ListObjectsV2 scans the bucket index, which does not scale on the billions-of-objects bucket (MAPCO-7954). GetTileKey/TileExists now issue a GetObjectMetadata (HEAD) — an O(1) key lookup — for each candidate extension in the historical Jpeg-then-Png order, resolving existence and the real key without touching the index. HEAD 404 (not the GET "NoSuchKey") drives miss detection. Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/Clients/S3Client.cs | 63 +++++++++++++++++++---- MergerLogicUnitTests/Utils/S3UtilsTest.cs | 46 +++++++++++------ 2 files changed, 82 insertions(+), 27 deletions(-) diff --git a/MergerLogic/Clients/S3Client.cs b/MergerLogic/Clients/S3Client.cs index f6992de5..e47e5446 100644 --- a/MergerLogic/Clients/S3Client.cs +++ b/MergerLogic/Clients/S3Client.cs @@ -5,7 +5,7 @@ using MergerLogic.ImageProcessing; using MergerLogic.Utils; using Microsoft.Extensions.Logging; -using System.Linq; +using System.Net; using System.Reflection; namespace MergerLogic.Clients @@ -29,6 +29,10 @@ public S3Client(IAmazonS3 client, IPathUtils pathUtils, IGeoUtils geoUtils, ILog this._storageClass = new S3StorageClass(storageClass ?? S3StorageClass.Standard); } + // Read paths don't know a tile's extension ahead of time, so existence is probed against + // these candidates in order (matches the historical Jpeg-then-Png GetTile lookup). + private static readonly TileFormat[] _readFormats = { TileFormat.Jpeg, TileFormat.Png }; + private bool IsKeyError(Exception e) { if (e is AmazonS3Exception ex) @@ -40,10 +44,22 @@ private bool IsKeyError(Exception e) { return en.ErrorCode == "NoSuchKey"; } - + return false; } + // HEAD returns 404 (not the GET "NoSuchKey") for a missing object. + private bool IsKeyNotFound(Exception e) + { + AmazonS3Exception? ex = e as AmazonS3Exception ?? e.InnerException as AmazonS3Exception; + if (ex is null) + { + return false; + } + + return ex.StatusCode == HttpStatusCode.NotFound || ex.ErrorCode == "NoSuchKey"; + } + private byte[]? GetImageBytes(string key) { string? methodName = MethodBase.GetCurrentMethod()?.Name; @@ -150,17 +166,44 @@ public void UpdateTile(Tile tile) this._logger.LogDebug($"[{methodName}] end {tile.ToString()}"); } + // Resolves existence and the real extension with a HEAD per candidate format. HEAD is an + // O(1) key lookup; a prefix LIST would scan the bucket index, which does not scale on the + // billions-of-objects bucket (MAPCO-7954). private string? GetTileKey(int z, int x, int y) + { + foreach (TileFormat format in _readFormats) + { + string key = this._pathUtils.GetTilePath(this.path, z, x, y, format, true); + if (this.KeyExists(key)) + { + return key; + } + } + + return null; + } + + private bool KeyExists(string key) { string methodName = MethodBase.GetCurrentMethod().Name; - string keyPrefix = this._pathUtils.GetTilePathWithoutExtension(this.path, z, x, y, true); - - // A single prefixed LIST (MaxKeys=1) resolves existence and the real extension without - // downloading the object body the way GetObject did. - var listRequest = new ListObjectsV2Request { BucketName = this._bucket, Prefix = keyPrefix, MaxKeys = 1 }; - this._logger.LogDebug($"[{methodName}] ListObjectsV2Async BucketName: {this._bucket}, Prefix: {keyPrefix}"); - var listObjectsTask = this._client.ListObjectsV2Async(listRequest); - return listObjectsTask.Result.S3Objects.FirstOrDefault()?.Key; + try + { + var request = new GetObjectMetadataRequest { BucketName = this._bucket, Key = key }; + this._logger.LogDebug($"[{methodName}] GetObjectMetadataAsync BucketName: {this._bucket}, Key: {key}"); + var task = this._client.GetObjectMetadataAsync(request); + _ = task.Result; + return true; + } + catch (AggregateException e) + { + if (IsKeyNotFound(e)) + { + this._logger.LogDebug($"[{methodName}] key not found: {key}"); + return false; + } + // In case there are other errors such as connection to S3 + throw e; + } } } } diff --git a/MergerLogicUnitTests/Utils/S3UtilsTest.cs b/MergerLogicUnitTests/Utils/S3UtilsTest.cs index 757cd2c2..9bae66df 100644 --- a/MergerLogicUnitTests/Utils/S3UtilsTest.cs +++ b/MergerLogicUnitTests/Utils/S3UtilsTest.cs @@ -1,4 +1,5 @@ -using Amazon.S3; +using Amazon.Runtime; +using Amazon.S3; using Amazon.S3.Model; using MergerLogic.Batching; using MergerLogic.Clients; @@ -12,6 +13,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Net; using System.Threading; using System.Threading.Tasks; @@ -324,32 +326,42 @@ public void GetTile(bool exist, GetTileParamType paramType, TileFormat tileForma [DataRow(false)] public void TileExists(bool exist) { - var seq = new MockSequence(); + var notFound = new AmazonS3Exception("Not Found", ErrorType.Sender, "NotFound", "req", HttpStatusCode.NotFound); + + // Jpeg is probed first via HEAD; only when it 404s is the Png candidate probed. this._pathUtilsMock - .InSequence(seq) - .Setup(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true)) - .Returns("key"); + .Setup(utils => utils.GetTilePath("test", 0, 0, 0, TileFormat.Jpeg, true)) + .Returns("keyJpeg"); + this._amazonS3ClientMock + .Setup(s3 => s3.GetObjectMetadataAsync( + It.Is(req => req.BucketName == "bucket" && req.Key == "keyJpeg"), + It.IsAny())) + .Returns(exist + ? Task.FromResult(new GetObjectMetadataResponse()) + : Task.FromException(notFound)); - var listResponse = new ListObjectsV2Response(); - if (exist) + if (!exist) { - listResponse.S3Objects.Add(new S3Object() { Key = "key" }); + this._pathUtilsMock + .Setup(utils => utils.GetTilePath("test", 0, 0, 0, TileFormat.Png, true)) + .Returns("keyPng"); + this._amazonS3ClientMock + .Setup(s3 => s3.GetObjectMetadataAsync( + It.Is(req => req.BucketName == "bucket" && req.Key == "keyPng"), + It.IsAny())) + .Returns(Task.FromException(notFound)); } - this._amazonS3ClientMock - .InSequence(seq) - .Setup(s3 => s3.ListObjectsV2Async(It.Is(req => - req.BucketName == "bucket" && req.Prefix == "key" && req.MaxKeys == 1), - It.IsAny())) - .ReturnsAsync(listResponse); var s3Utils = new S3Client(this._amazonS3ClientMock.Object, this._pathUtilsMock.Object, this._geoUtilsMock.Object, this._loggerMock.Object, "STANDARD", "bucket", "test"); Assert.AreEqual(exist, s3Utils.TileExists(0, 0, 0)); - this._pathUtilsMock.Verify(utils => utils.GetTilePathWithoutExtension("test", 0, 0, 0, true), Times.Once); - this._amazonS3ClientMock.Verify(s3 => s3.ListObjectsV2Async(It.Is(req => - req.BucketName == "bucket" && req.Prefix == "key" && req.MaxKeys == 1), It.IsAny()), Times.Once); + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectMetadataAsync( + It.Is(req => req.Key == "keyJpeg"), It.IsAny()), Times.Once); + this._amazonS3ClientMock.Verify(s3 => s3.GetObjectMetadataAsync( + It.Is(req => req.Key == "keyPng"), It.IsAny()), + exist ? Times.Never() : Times.Once()); this.VerifyAll(); }