Closed waahm7 closed 7 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
237e9e1
) 89.10% compared to head (ba87de4
) 89.10%.
could we benchmark on something like c5n.18xlarge that has NUMA and a 100Gbit NIC? that's where I would (naively) expect the pinning effect to show up, if anywhere.
@jamesbornholt Thanks, that's a great suggestion. I have added some benchmarks on a c5n.18xlarge and haven't observed any performance degradation.
Description of changes: We try to pin the body streaming ELG to CPU group 1 if multiple CPU groups are available. On a
c5.24xlarge
EC2 instance, we have the following CPU groups:We try to pin our body streaming ELG to the CPUs of node 1, which range from 24 to 95 in CPU IDs. At this location, we attempt to create a thread pinned to each CPU ID from the list. However, some CPUs might be restricted for the process. If any of the CPU IDs is restricted,
pthread_create
fails for that particular CPU ID here with an error code of 22 (EINVAL = Invalid Argument error), which is treated as a fatal error in our codebase.I have run some benchmarks with and without CPU group pinning on a
c5.24xlarge
and observed no difference in performance. Therefore, this PR disables CPU group pinning. Here are the benchmark results:Benchmarks on c5n.18xlarge:
This PR doesn't address the use case of customers creating a pinned ELG. It simply simplifies the S3 codebase until we have a better reason to integrate CPU group pinning into S3, other than the assumption of performance improvement. I will create another PR which updates the aws_thread_launch API to implement best-effort pinning of the CPU_ID.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.