Add support for C++ benchmarks. #99
Conversation
WalkthroughThis change introduces first-class C++ (cpp) support throughout the serverless benchmarking system, especially for AWS Lambda. It adds C++ benchmark code, Docker build scripts for C++ dependencies, and wrappers for AWS Lambda in C++. The codebase is refactored to use a strongly-typed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Benchmark
participant DockerBuilder
participant AWS
participant Lambda
User->>CLI: Run benchmark with --language cpp
CLI->>Benchmark: Prepare C++ benchmark
Benchmark->>DockerBuilder: Build C++ Lambda package (CMake)
DockerBuilder->>AWS: Package and deploy C++ function
AWS->>Lambda: Create function with C++ runtime
User->>Lambda: Invoke C++ Lambda function
Lambda->>User: Return result
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (4)
benchmarks/wrappers/aws/cpp/redis.hpp (1)
1-27: Add include guards and improve class design.The header file lacks include guards and has several design issues that should be addressed:
- Missing include guards: The header should have include guards to prevent multiple inclusions
- Constructor should be explicit: Single-parameter constructors should be explicit to prevent implicit conversions
- Missing const correctness: Methods that don't modify state should be const
- Raw pointer management: Consider RAII principles for
redisContext*+#ifndef REDIS_HPP +#define REDIS_HPP + #include <cstdint> #include <string> #include <aws/core/utils/memory/stl/AWSString.h> #include <hiredis/hiredis.h> class Redis { redisContext* _context; public: - Redis(std::string redis_hostname, int redis_port); + explicit Redis(const std::string& redis_hostname, int redis_port); ~Redis(); - bool is_initialized(); + bool is_initialized() const; - uint64_t download_file(Aws::String const &key, int &required_retries, bool with_backoff); + uint64_t download_file(const Aws::String& key, int& required_retries, bool with_backoff) const; - uint64_t upload_file(Aws::String const &key, int size, char* pBuf); + uint64_t upload_file(const Aws::String& key, int size, const char* pBuf); - uint64_t delete_file(std::string const &key); + uint64_t delete_file(const std::string& key) const; }; +#endif // REDIS_HPPbenchmarks/wrappers/aws/cpp/key-value.hpp (1)
1-32: Add include guards and improve interface design.The header file has several areas for improvement:
- Missing include guards: Add them to prevent multiple inclusions
- Const correctness: Methods that don't modify state should be const
- Parameter const-correctness: Input parameters should be const references
- Comment about non-copyable/non-movable: Should be enforced with deleted copy/move operators
+#ifndef KEY_VALUE_HPP +#define KEY_VALUE_HPP + #include <cstdint> #include <string> #include <initializer_list> #include <memory> #include <aws/core/utils/memory/stl/AWSString.h> #include <aws/dynamodb/DynamoDBClient.h> class KeyValue { - // non-copyable, non-movable std::shared_ptr<Aws::DynamoDB::DynamoDBClient> _client; public: KeyValue(); + + // non-copyable, non-movable + KeyValue(const KeyValue&) = delete; + KeyValue& operator=(const KeyValue&) = delete; + KeyValue(KeyValue&&) = delete; + KeyValue& operator=(KeyValue&&) = delete; -uint64_t download_file(Aws::String const &bucket, - Aws::String const &key, +uint64_t download_file(const Aws::String& bucket, + const Aws::String& key, int& required_retries, double& read_units, - bool with_backoff = false); + bool with_backoff = false) const; -uint64_t upload_file(Aws::String const &bucket, - Aws::String const &key, +uint64_t upload_file(const Aws::String& bucket, + const Aws::String& key, double& write_units, int size, - unsigned char* pBuf); + const unsigned char* pBuf); }; +#endif // KEY_VALUE_HPPbenchmarks/wrappers/aws/cpp/storage.hpp (1)
1-29: Add include guards and improve interface consistency.The header file needs several improvements:
- Missing include guards: Add them for safety
- Interface consistency: Consider aligning parameter patterns with other wrapper classes
- Const correctness: Download method should be const
+#ifndef STORAGE_HPP +#define STORAGE_HPP + #include <cstdint> #include <aws/core/utils/memory/stl/AWSString.h> #include <aws/s3/S3Client.h> class Storage { Aws::S3::S3Client _client; public: - Storage(Aws::S3::S3Client && client): + explicit Storage(Aws::S3::S3Client&& client): _client(client) {} static Storage get_client(); - uint64_t download_file(Aws::String const &bucket, - Aws::String const &key, + uint64_t download_file(const Aws::String& bucket, + const Aws::String& key, int &required_retries, - bool report_dl_time); + bool report_dl_time) const; - uint64_t upload_random_file(Aws::String const &bucket, - Aws::String const &key, - int size, char* pBuf); + uint64_t upload_random_file(const Aws::String& bucket, + const Aws::String& key, + int size, const char* pBuf); }; +#endif // STORAGE_HPPtools/build_docker_images.py (1)
51-151: CRITICAL: Resolve Git merge conflicts before merging.The file contains unresolved Git merge conflict markers (
<<<<<<<,=======,>>>>>>>) that cause syntax errors and prevent the code from running. These conflicts must be resolved manually.The conflicts appear to be between:
- HEAD version with dot notation:
f"{args.type}.{args.type_tag}"- Branch a9f3c27 with hyphen notation:
f"{args.type}-{args.type_tag}"Please resolve these conflicts by choosing the appropriate format and removing all conflict markers.
🧹 Nitpick comments (18)
dockerfiles/aws/nodejs/Dockerfile.build (1)
5-5: Optimize yum layer by cleaning cache.To slim down the final image, combine the install and cleanup steps into a single
RUNand clear the yum cache:-RUN yum install -y cmake curl libcurl libcurl-devel +RUN yum install -y cmake curl libcurl libcurl-devel && \ + yum clean all && \ + rm -rf /var/cache/yumdockerfiles/aws/cpp/Dockerfile.dependencies-hiredis (1)
7-8: Consider adding error handling and cleanup.The build process could benefit from explicit error handling and cleanup to ensure robust builds and smaller image sizes.
-RUN git clone https://github.com/redis/hiredis.git && cd hiredis\ - && PREFIX=/opt make -j${WORKERS} install +RUN set -e && \ + git clone --depth 1 --branch v1.2.0 https://github.com/redis/hiredis.git && \ + cd hiredis && \ + PREFIX=/opt make -j${WORKERS} install && \ + cd .. && \ + rm -rf hiredisbenchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp (2)
7-7: Remove unused include.The
iostreamheader is included but never used in this file.#include <thread> -#include <iostream>
16-17: Remove commented-out code.Commented-out code should be removed to keep the codebase clean. If this alternative implementation is needed for reference, consider documenting it properly or moving it to documentation.
- //std::string res_json = "{ \"result\": " + std::to_string(sleep) + "}"; - //return aws::lambda_runtime::invocation_response::success(res_json, "application/json");dockerfiles/aws/cpp/Dockerfile.dependencies-runtime (1)
8-10: Consider improving build robustness and cleanup.The build process could benefit from better error handling and cleanup to reduce image size and improve reliability.
-RUN git clone https://github.com/awslabs/aws-lambda-cpp.git\ - && cmake3 -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DCMAKE_INSTALL_PREFIX=/opt -B aws-lambda-cpp/build -S aws-lambda-cpp\ - && cmake3 --build aws-lambda-cpp/build --parallel ${WORKERS} --target install +RUN set -e && \ + git clone --depth 1 --branch v0.2.8 https://github.com/awslabs/aws-lambda-cpp.git && \ + cmake3 -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DCMAKE_INSTALL_PREFIX=/opt \ + -B aws-lambda-cpp/build -S aws-lambda-cpp && \ + cmake3 --build aws-lambda-cpp/build --parallel ${WORKERS} --target install && \ + rm -rf aws-lambda-cppdockerfiles/aws/cpp/Dockerfile.dependencies-sdk (1)
9-9: Consider adding integrity verification and optimizing cmake configuration.The cmake configuration looks appropriate, but consider adding integrity checks and optimization flags.
&& cmake3 -DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;dynamodb;sqs" -DENABLE_TESTING=OFF -DCMAKE_INSTALL_PREFIX=/opt/ ..\ + && cmake3 -DCMAKE_BUILD_TYPE=Release -DBUILD_ONLY="s3;dynamodb;sqs" -DENABLE_TESTING=OFF -DCMAKE_INSTALL_PREFIX=/opt/ -DCPP_STANDARD=17 -DCMAKE_CXX_FLAGS="-O3" ..\dockerfiles/aws/cpp/Dockerfile.dependencies-boost (1)
13-15: Remove commented code.The commented lines should be removed as they add confusion and don't serve any purpose in the final implementation.
-#RUN curl -LO https://boostorg.jfrog.io/artifactory/main/release/1.79.0/source/boost_1_79_0.tar.gz\ -# && tar -xf boost_1_79_0.tar.gz && cd boost_1_79_0 && ./bootstrap.sh --prefix=/opt\ -# && ./b2 -j${WORKERS} --prefix=/opt cxxflags="-fPIC" link=static installconfig/systems.json (1)
125-137: Consider explicit architecture specification for consistency.The C++ configuration uses
"all"for architecture while Python and Node.js specify explicit architectures ("x64","arm64"). This inconsistency might cause confusion and could limit architecture-specific optimizations.Consider updating to be consistent with other languages:
"cpp": { "base_images": { - "all": "amazon/aws-lambda-provided:al2.2022.04.27.09" + "x64": "amazon/aws-lambda-provided:al2.2022.04.27.09", + "arm64": "amazon/aws-lambda-provided:al2.2022.04.27.09" }, "dependencies": [ "runtime", "sdk", "boost", "hiredis" ], - "versions": ["all"], + "versions": ["all"], "images": ["build"],dockerfiles/aws/cpp/Dockerfile.build (1)
18-21: Optimize script copying and permission setting.Consider combining COPY and chmod operations to reduce layers.
-COPY docker/entrypoint.sh /sebs/entrypoint.sh -COPY docker/cpp_installer.sh /sebs/installer.sh -RUN chmod +x /sebs/entrypoint.sh -RUN chmod +x /sebs/installer.sh +COPY --chmod=755 docker/entrypoint.sh /sebs/entrypoint.sh +COPY --chmod=755 docker/cpp_installer.sh /sebs/installer.shbenchmarks/wrappers/aws/cpp/redis.hpp (1)
11-11: Consider using smart pointers for automatic resource management.The raw
redisContext*pointer requires manual memory management. Consider usingstd::unique_ptr<redisContext, void(*)(redisContext*)>with a custom deleter for automatic cleanup.+#include <memory> + class Redis { - redisContext* _context; + std::unique_ptr<redisContext, void(*)(redisContext*)> _context; public:benchmarks/wrappers/aws/cpp/storage.hpp (1)
18-26: Consider interface consistency across wrapper classes.The method signatures differ from other wrapper classes (Redis, KeyValue). Consider standardizing parameter patterns for consistency, especially for retry handling and timing measurements.
For example, the Redis class uses
int& required_retriesandbool with_backoff, while this class usesbool report_dl_time. Consider aligning these interfaces for consistency across the AWS wrapper ecosystem.benchmarks/wrappers/aws/cpp/key-value.cpp (1)
19-20: Consider making configuration more flexible.The hardcoded CA file path and commented region configuration reduce flexibility across different environments.
Consider reading these from environment variables:
- //config.region = "eu-central-1"; - config.caFile = "/etc/pki/tls/certs/ca-bundle.crt"; + const char* region = std::getenv("AWS_REGION"); + if (region) { + config.region = region; + } + const char* caFile = std::getenv("AWS_CA_BUNDLE"); + config.caFile = caFile ? caFile : "/etc/pki/tls/certs/ca-bundle.crt";benchmarks/wrappers/aws/cpp/handler.cpp (1)
23-23: Fix typo in comment."Gateaway" should be "Gateway".
- // HTTP trigger with API Gateaway sends payload as a serialized JSON + // HTTP trigger with API Gateway sends payload as a serialized JSONbenchmarks/wrappers/aws/cpp/storage.cpp (3)
46-50: Clarify the NOP operation purpose.The current implementation to prevent compiler optimization is unusual. Consider a simpler approach or document why this specific pattern is needed.
// Perform NOP on result to prevent optimizations std::stringstream ss; ss << s.rdbuf(); - std::string first(" "); - ss.get(&first[0], 1); + // Simply access the stream to prevent optimization + volatile auto size = ss.tellp(); + (void)size; // Suppress unused variable warning
59-63: Remove commented code or document why it's preserved.The commented backoff logic should either be removed or documented if it's intentionally kept for future use.
} else { retries += 1; - //int sleep_time = retries; - //if (retries > 100) { - // sleep_time = retries * 2; - //} - //std::this_thread::sleep_for(std::chrono::milliseconds(sleep_time)); + // TODO: Consider adding backoff delay for failed downloads }
75-78: Simplify the comment about bufferstream usage.The comment is quite long and could be more concise while still explaining the rationale.
- /** - * We use Boost's bufferstream to wrap the array as an IOStream. Usign a light-weight streambuf wrapper, as many solutions - * (e.g. https://stackoverflow.com/questions/13059091/creating-an-input-stream-from-constant-memory) on the internet - * suggest does not work because the S3 SDK relies on proper functioning tellp(), etc... (for instance to get the body length). - */ + // Use Boost's bufferstream to wrap the buffer as an IOStream. + // The S3 SDK requires proper tellp() functionality, so simple streambuf wrappers don't work.sebs/aws/aws.py (1)
671-677: Simplify the conditional structure.The
elifafterreturnis unnecessary and can be simplified for better readability.def cloud_runtime(self, language: Language, language_version: str): if language in [Language.NODEJS, Language.PYTHON]: return ("{}{}".format(language, language_version),) - elif language == Language.CPP: + if language == Language.CPP: return "provided.al2" - else: - raise NotImplementedError() + raise NotImplementedError()sebs/benchmark.py (1)
18-19: Clean up duplicate import.Remove the duplicate
Languageimport on line 19.from sebs.utils import find_benchmark, project_absolute_path, LoggingBase -from sebs.types import BenchmarkModule, Language from sebs.types import Language
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
benchmarks/000.microbenchmarks/010.sleep/config.json(1 hunks)benchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp(1 hunks)benchmarks/wrappers/aws/cpp/handler.cpp(1 hunks)benchmarks/wrappers/aws/cpp/key-value.cpp(1 hunks)benchmarks/wrappers/aws/cpp/key-value.hpp(1 hunks)benchmarks/wrappers/aws/cpp/redis.cpp(1 hunks)benchmarks/wrappers/aws/cpp/redis.hpp(1 hunks)benchmarks/wrappers/aws/cpp/storage.cpp(1 hunks)benchmarks/wrappers/aws/cpp/storage.hpp(1 hunks)benchmarks/wrappers/aws/cpp/utils.cpp(1 hunks)benchmarks/wrappers/aws/cpp/utils.hpp(1 hunks)config/systems.json(1 hunks)dockerfiles/aws/cpp/Dockerfile.build(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-boost(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-hiredis(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-runtime(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-sdk(1 hunks)dockerfiles/aws/nodejs/Dockerfile.build(1 hunks)dockerfiles/cpp_installer.sh(1 hunks)sebs.py(1 hunks)sebs/aws/aws.py(6 hunks)sebs/azure/azure.py(4 hunks)sebs/benchmark.py(11 hunks)sebs/faas/container.py(9 hunks)sebs/faas/function.py(9 hunks)sebs/faas/system.py(2 hunks)sebs/gcp/gcp.py(4 hunks)sebs/openwhisk/openwhisk.py(3 hunks)sebs/types.py(2 hunks)tools/build_docker_images.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
sebs/faas/container.py
[refactor] 143-143: Too many arguments (7/5)
(R0913)
[refactor] 143-143: Too many positional arguments (7/5)
(R0917)
[refactor] 143-143: Too many local variables (19/15)
(R0914)
tools/build_docker_images.py
[error] 51-51: Parsing failed: 'invalid syntax (build_docker_images, line 51)'
(E0001)
sebs/aws/aws.py
[refactor] 672-677: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
🪛 Ruff (0.11.9)
sebs/faas/function.py
251-253: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tools/build_docker_images.py
51-51: SyntaxError: Expected a statement
51-51: SyntaxError: Expected a statement
51-51: SyntaxError: Expected a statement
51-51: SyntaxError: Expected a statement
52-52: SyntaxError: Unexpected indentation
53-53: SyntaxError: Expected a statement
53-53: SyntaxError: Expected a statement
53-53: SyntaxError: Expected a statement
53-53: SyntaxError: Expected a statement
53-54: SyntaxError: Expected a statement
54-54: SyntaxError: Unexpected indentation
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected a statement
59-59: SyntaxError: Expected ',', found name
59-59: SyntaxError: Expected ',', found name
59-59: SyntaxError: Expected ',', found name
109-109: SyntaxError: Expected a statement
109-109: SyntaxError: Expected a statement
109-109: SyntaxError: Expected a statement
109-109: SyntaxError: Expected a statement
111-111: SyntaxError: Unexpected indentation
136-136: SyntaxError: unindent does not match any outer indentation level
136-136: SyntaxError: Expected a statement
136-137: SyntaxError: Expected a statement
137-137: SyntaxError: Unexpected indentation
138-138: SyntaxError: Expected a statement
138-138: SyntaxError: Expected a statement
138-138: SyntaxError: Expected a statement
138-138: SyntaxError: Expected a statement
138-139: SyntaxError: Expected a statement
140-140: SyntaxError: Unexpected indentation
149-149: SyntaxError: unindent does not match any outer indentation level
149-149: SyntaxError: Expected a statement
149-150: SyntaxError: Expected a statement
150-150: SyntaxError: Unexpected indentation
151-151: SyntaxError: Expected a statement
151-151: SyntaxError: Expected a statement
151-151: SyntaxError: Expected a statement
151-151: SyntaxError: Expected a statement
151-151: SyntaxError: Expected ',', found name
151-151: SyntaxError: Expected ',', found name
151-151: SyntaxError: Expected ',', found name
152-152: SyntaxError: Unexpected indentation
152-152: SyntaxError: Expected a statement
152-152: SyntaxError: Expected a statement
152-153: SyntaxError: Expected a statement
153-153: SyntaxError: Unexpected indentation
sebs/benchmark.py
19-19: Redefinition of unused Language from line 18
Remove definition: Language
(F811)
🔇 Additional comments (33)
dockerfiles/aws/nodejs/Dockerfile.build (1)
4-5: Update comment and review package dependencies for user management.The
# useradd, groupmodcomment no longer matches the installed packages—you're now installing C++ build tools (cmake,curl,libcurl,libcurl-devel) but notshadow-utils(which providesuseradd/groupmod). If you still rely ongosuto switch users, either re-addshadow-utilsto the install line or remove/update the comment to reflect the new purpose.sebs/types.py (1)
45-57: Well-structured Architecture enum implementation.The Architecture enum with its serialize/deserialize methods follows good patterns and provides type safety for the codebase.
benchmarks/000.microbenchmarks/010.sleep/config.json (1)
4-4: LGTM - Consistent language addition.The addition of "cpp" to the languages array is consistent with the existing configuration pattern and enables C++ benchmark support.
sebs.py (1)
67-67: LGTM - Correct CLI integration.The addition of "cpp" to the language choices properly integrates C++ support into the command-line interface.
sebs/faas/system.py (1)
16-16: LGTM! Excellent type safety improvement.The addition of the
Languageenum import and updating the method signature to use strongly-typedLanguageinstead of string-basedlanguage_namesignificantly improves type safety and code maintainability. This change aligns well with the broader refactoring across the codebase.Also applies to: 174-174
dockerfiles/aws/cpp/Dockerfile.dependencies-runtime (1)
9-9: Approve the CMake configuration choices.The CMake configuration looks well-thought-out:
Releasebuild type for optimized binariesBUILD_SHARED_LIBS=OFFfor static linking (appropriate for Lambda)- Custom install prefix to
/optfor multi-stage builds- Parallel builds using
${WORKERS}for build efficiencyconfig/systems.json (1)
129-131: Verify dependency order and completeness.The dependencies list looks comprehensive, but ensure the order matches any build dependencies (e.g., runtime before SDK).
#!/bin/bash # Verify that all dependency Dockerfiles exist for the listed dependencies echo "Checking for dependency Dockerfiles..." for dep in runtime sdk boost hiredis; do dockerfile="dockerfiles/aws/cpp/Dockerfile.dependencies-${dep}" if [ -f "$dockerfile" ]; then echo "✓ Found: $dockerfile" else echo "✗ Missing: $dockerfile" fi donesebs/gcp/gcp.py (3)
27-27: Good refactoring for type safety.The import of the
Languageenum improves type safety and consistency across the codebase.
142-147: Type-safe dictionary access implementation looks correct.The refactoring properly updates both
CONFIG_FILESandHANDLERdictionaries to useLanguageenum keys, and all access patterns use the typedlanguageparameter.
128-148: I didn’t locate the PythonLanguageenum – let’s search for its class definition and inspect it for aCPPmember:#!/bin/bash # Locate and display the Python enum for Language echo "Searching for Language enum in .py files..." rg -n "class Language" -g "*.py" echo -e "\nShowing definition and first 20 lines after declaration:" file=$(rg -l "class Language" -g "*.py" | head -n1) if [[ -n "$file" ]]; then start_line=$(grep -n "class Language" "$file" | head -n1 | cut -d: -f1) sed -n "${start_line},$((start_line+20))p" "$file" else echo "❌ Language enum definition file not found." fidockerfiles/aws/cpp/Dockerfile.build (2)
23-26: Dependency aggregation looks correct.The sequential copying from all dependency stages properly aggregates the built libraries and headers into
/opt.
30-31: ```shell
#!/bin/bash
echo "Locating Dockerfile.build files..."
fd --type f 'Dockerfile.build'echo
echo "Printing lines 1–60 of dockerfiles/aws/cpp/Dockerfile.build:"
sed -n '1,60p' dockerfiles/aws/cpp/Dockerfile.buildecho
echo "Looking for any COPY commands or /sebs references:"
grep -R -n 'COPY|sebs' dockerfiles/aws/cpp/Dockerfile.build</details> <details> <summary>sebs/openwhisk/openwhisk.py (3)</summary> `21-21`: **Good addition of type safety with Language enum import.** The import of `Language` enum enhances type safety and consistency across the codebase. --- `100-100`: **Parameter type change improves type safety.** Changing from `language_name: str` to `language: Language` provides compile-time type checking and prevents invalid language strings. --- `111-111`: **Correct usage of language.value for backward compatibility.** Using `language.value` maintains compatibility with downstream methods expecting string identifiers. </details> <details> <summary>sebs/azure/azure.py (3)</summary> `27-27`: **Good addition of Language enum import for type safety.** The import enhances type consistency across cloud provider modules. --- `122-122`: **Parameter type change improves type safety.** The change from string to `Language` enum parameter provides better type checking. --- `155-155`: **Dictionary lookup now uses proper enum type.** Good use of the Language enum for dictionary access instead of string. </details> <details> <summary>benchmarks/wrappers/aws/cpp/key-value.hpp (1)</summary> `13-13`: **Good use of shared_ptr for AWS client management.** Using `shared_ptr` for the DynamoDB client is appropriate as it handles the AWS SDK object lifecycle correctly. </details> <details> <summary>benchmarks/wrappers/aws/cpp/storage.hpp (2)</summary> `12-14`: **Good use of move semantics in constructor.** The constructor correctly uses move semantics to efficiently transfer ownership of the S3Client, avoiding unnecessary copies. --- `16-16`: **Static factory method follows good design patterns.** The `get_client()` static factory method is a good design choice for encapsulating client creation logic. </details> <details> <summary>sebs/faas/container.py (3)</summary> `12-12`: **LGTM: Type-safe language handling.** The import of the `Language` enum improves type safety and consistency across the codebase. --- `143-151`: **LGTM: Method signature update for type safety.** The change from `language_name: str` to `language: Language` parameter improves type safety and aligns with the broader refactoring to use strongly-typed language identifiers. --- `164-164`: **LGTM: Consistent use of Language enum.** The usage of `language.value` to extract the string representation is correct and consistent throughout the method. Also applies to: 188-188, 201-201 </details> <details> <summary>sebs/aws/aws.py (3)</summary> `25-25`: **LGTM: Import addition aligns with type safety improvements.** The import of the `Language` enum from `sebs.types` supports the refactoring to use strongly-typed language identifiers throughout the codebase. --- `121-121`: **LGTM: Parameter type improved with enum.** Changing the parameter type from string to `Language` enum enhances type safety and prevents invalid language values. --- `139-175`: ```bash #!/bin/bash set -eo pipefail # 1. List all files under benchmarks/ to inspect their extensions find benchmarks/ -type f # 2. Search for any 'zip' commands or references to 'benchmark.zip' in those files grep -R --include="*.sh" --include="Makefile" --include="*.py" -nE "zip|benchmark\.zip" benchmarks/ || true # 3. Search for any build directory creations or references grep -R -nE "mkdir.*build|cmake|make" benchmarks/ || truesebs/faas/function.py (3)
12-12: LGTM: Centralized enum imports improve consistency.Importing
LanguageandArchitecturefrom the sharedsebs.typesmodule enhances modularity and ensures consistent enum usage across the codebase.
221-227: LGTM: Enhanced JSON parsing for C++ triggers.The enhanced parsing logic correctly handles AWS C++ trigger responses that may have nested JSON bodies, improving compatibility with the new C++ runtime.
290-292: LGTM: Consistent use of enum deserialization.The update to use
Language.deserialize()maintains consistency with the typed enum approach throughout the codebase.sebs/benchmark.py (3)
7-7: LGTM: Appropriate import for CMake script formatting.The
textwrapimport is correctly added to support the dedenting of the CMake script in the new C++ packaging method.
410-450: Well-structured C++ packaging method with future improvement opportunities.The CMake script generation is well-implemented and correctly handles C++ Lambda runtime dependencies. The FIXME comments appropriately identify areas for future enhancement.
Consider addressing the FIXME comments in future iterations:
- Generic configuration for different cloud providers
- Optional dependency configuration
- Dynamic dependency resolution
629-629: LGTM: Enhanced error logging for debugging.Adding stderr decoding to the Docker build error logging provides better diagnostic information when builds fail.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sebs/aws/aws.py (1)
676-682: Simplify the conditional structure.The
elifafterreturnis unnecessary and can be simplified for better readability.def cloud_runtime(self, language: Language, language_version: str): if language in [Language.NODEJS, Language.PYTHON]: return ("{}{}".format(language, language_version),) - elif language == Language.CPP: + if language == Language.CPP: return "provided.al2" - else: - raise NotImplementedError() + raise NotImplementedError()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sebs/aws/aws.py(6 hunks)sebs/benchmark.py(16 hunks)sebs/faas/container.py(6 hunks)sebs/faas/function.py(3 hunks)sebs/openwhisk/openwhisk.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sebs/openwhisk/openwhisk.py
- sebs/faas/function.py
🧰 Additional context used
🪛 Pylint (3.3.7)
sebs/aws/aws.py
[refactor] 677-682: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
sebs/faas/container.py
[refactor] 137-137: Too many arguments (7/5)
(R0913)
[refactor] 137-137: Too many positional arguments (7/5)
(R0917)
[refactor] 137-137: Too many local variables (19/15)
(R0914)
🔇 Additional comments (15)
sebs/faas/container.py (3)
12-12: LGTM: Good type safety improvement.The addition of the
Languageenum import supports the transition from string-based language handling to a more type-safe approach.
102-106: LGTM: Improved code formatting.The parameter formatting improves readability without changing functionality.
140-140: LGTM: Proper enum usage throughout the method.The method correctly uses
Languageenum type for the parameter andlanguage.valuewhen string representation is needed for Docker operations and file paths.Also applies to: 158-158, 181-181, 193-193
sebs/aws/aws.py (3)
25-25: LGTM: Type safety improvement.Adding the
Languageenum import supports better type safety throughout the codebase.
121-121: LGTM: Proper enum adoption.The method signature correctly uses the
Languageenum, and theCONFIG_FILESdictionary properly maps enum values to their respective configuration files.Also applies to: 144-146
148-180: LGTM: Well-structured C++ packaging logic.The conditional logic properly handles different languages. The C++ implementation assumes the build script creates a zip file in the build directory, which aligns with typical CMake build patterns for AWS Lambda.
sebs/benchmark.py (9)
7-7: LGTM: Required imports for C++ support.The
textwrapimport is needed for the CMakeLists.txt generation, and theLanguageenum import supports the type safety improvements.Also applies to: 18-18
30-30: LGTM: Proper type annotation.The type annotation correctly uses the
Languageenum for better type safety.
180-180: LGTM: Consistent enum usage in hash operations.The hash methods now properly use the
Languageenum, maintaining consistency with the broader refactoring.Also applies to: 253-253
257-282: LGTM: Comprehensive C++ file support.The file handling dictionaries properly include C++ file types (
*.cpp,*.hpp,dependencies.json) and integrate them consistently with existing Python and Node.js support.
327-334: LGTM: Updated copy logic for C++ files.The copy_code method correctly handles C++ files using the Language enum.
413-454: Well-implemented C++ CMakeLists.txt generation.The CMakeLists.txt generation includes all necessary dependencies for AWS Lambda C++ runtime:
- AWS Lambda runtime
- Boost libraries
- AWS SDK (S3, DynamoDB)
- Hiredis for Redis support
The FIXME comments appropriately mark areas for future enhancement (generic deployment support, optional dependencies).
541-546: LGTM: C++ package file configuration.Correctly identifies
CMakeLists.txtas the package file for C++ builds, consistent with CMake build systems.
632-632: LGTM: Improved error logging.Adding stderr output to error logging provides better debugging information when Docker builds fail.
643-643: LGTM: Updated method signature for type safety.The build method now properly uses the
Languageenum in its callable parameter type, maintaining consistency with the broader refactoring.Also applies to: 684-684
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
sebs/aws/aws.py (1)
168-185: Fix inconsistent return path for C++ packages.The C++ packaging logic creates the archive in
build/benchmark.zip(line 171) but the return statement (line 182) still uses the old path format{}.zipwhich doesn't account for the C++ build directory structure.Apply this fix:
+ elif language == Language.CPP: + benchmark_archive = os.path.join(directory, "build", "benchmark.zip") return ( - os.path.join(directory, "{}.zip".format(benchmark)), + benchmark_archive, bytes_size, container_uri, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
benchmarks/wrappers/aws/cpp/handler.cpp(1 hunks)benchmarks/wrappers/aws/cpp/key-value.cpp(1 hunks)benchmarks/wrappers/aws/cpp/redis.cpp(1 hunks)benchmarks/wrappers/aws/cpp/storage.cpp(1 hunks)benchmarks/wrappers/aws/cpp/utils.cpp(1 hunks)benchmarks/wrappers/aws/cpp/utils.hpp(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-hiredis(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-runtime(1 hunks)dockerfiles/aws/cpp/Dockerfile.dependencies-sdk(1 hunks)dockerfiles/cpp_installer.sh(1 hunks)sebs/aws/aws.py(6 hunks)sebs/types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- dockerfiles/aws/cpp/Dockerfile.dependencies-hiredis
- dockerfiles/aws/cpp/Dockerfile.dependencies-sdk
- dockerfiles/cpp_installer.sh
- dockerfiles/aws/cpp/Dockerfile.dependencies-runtime
- benchmarks/wrappers/aws/cpp/key-value.cpp
- sebs/types.py
- benchmarks/wrappers/aws/cpp/handler.cpp
- benchmarks/wrappers/aws/cpp/redis.cpp
- benchmarks/wrappers/aws/cpp/storage.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mcopik
PR: spcl/serverless-benchmarks#99
File: sebs/azure/azure.py:137-0
Timestamp: 2025-06-19T16:19:09.718Z
Learning: C++ support in the serverless-benchmarks project is currently limited to AWS Lambda only. Azure Functions implementation (sebs/azure/azure.py) does not support C++ and only supports Python and Node.js languages.
Learnt from: mcopik
PR: spcl/serverless-benchmarks#99
File: sebs/openwhisk/openwhisk.py:119-0
Timestamp: 2025-06-19T16:18:56.851Z
Learning: C++ language support in the serverless benchmarks framework is not available for OpenWhisk platform. C++ support is limited to specific platforms like AWS.
🪛 Pylint (3.3.7)
sebs/aws/aws.py
[refactor] 197-204: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
🔇 Additional comments (2)
benchmarks/wrappers/aws/cpp/utils.cpp (1)
6-10: LGTM! Naming inconsistency has been resolved.The function name
timeSinceEpochMicrosecnow correctly reflects that it returns microseconds, matching the implementation. This addresses the previous naming inconsistency issue.benchmarks/wrappers/aws/cpp/utils.hpp (1)
1-8: LGTM! Include guards have been properly added.The header file now includes proper include guards and the necessary
<cstdint>header, addressing the previous review comment about multiple inclusion prevention.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sebs/aws/aws.py (1)
193-204: LGTM: C++ runtime support added correctly.The method properly handles the new C++ language with the appropriate AWS Lambda custom runtime
"provided.al2".Consider addressing the style issue flagged by static analysis - the
elifstatements afterreturnare unnecessary:def cloud_runtime(self, language: Language, language_version: str): # AWS uses different naming scheme for Node.js versions # For example, it's 12.x instead of 12. if language == Language.NODEJS: return f"{language}{language_version}.x" - elif language == Language.CPP: + if language == Language.CPP: return "provided.al2" - elif language in [Language.PYTHON]: + if language == Language.PYTHON: return f"{language}{language_version}" - else: + raise NotImplementedError()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sebs/aws/aws.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mcopik
PR: spcl/serverless-benchmarks#99
File: sebs/azure/azure.py:137-0
Timestamp: 2025-06-19T16:19:09.718Z
Learning: C++ support in the serverless-benchmarks project is currently limited to AWS Lambda only. Azure Functions implementation (sebs/azure/azure.py) does not support C++ and only supports Python and Node.js languages.
Learnt from: mcopik
PR: spcl/serverless-benchmarks#99
File: sebs/openwhisk/openwhisk.py:119-0
Timestamp: 2025-06-19T16:18:56.851Z
Learning: C++ language support in the serverless benchmarks framework is not available for OpenWhisk platform. C++ support is limited to specific platforms like AWS.
🪛 Pylint (3.3.7)
sebs/aws/aws.py
[refactor] 197-204: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
🔇 Additional comments (5)
sebs/aws/aws.py (5)
25-25: LGTM: Language enum import aligns with system-wide type safety improvements.The import of the
Languageenum supports the transition from string-based to strongly-typed language handling throughout the codebase.
118-127: LGTM: Method signature updated to use Language enum.The transition from string-based to enum-based language parameter improves type safety and consistency across the codebase.
144-146: LGTM: CONFIG_FILES dictionary updated for enum-based language handling.The dictionary keys are properly updated to use Language enum values instead of strings.
216-216: LGTM: Correctly using Language enum from code package.The change from string-based to enum-based language access is consistent with the type-safe approach introduced in this PR.
281-281: LGTM: Consistent enum usage in runtime configuration.The method call correctly passes the Language enum to the updated
cloud_runtimemethod signature.
| if language in [Language.PYTHON, Language.NODEJS]: | ||
| package_config = CONFIG_FILES[language] | ||
| function_dir = os.path.join(directory, "function") | ||
| os.makedirs(function_dir) | ||
| # move all files to 'function' except handler.py | ||
| for file in os.listdir(directory): | ||
| if file not in package_config: | ||
| file = os.path.join(directory, file) | ||
| shutil.move(file, function_dir) | ||
|
|
||
| # FIXME: use zipfile | ||
| # create zip with hidden directory but without parent directory | ||
| execute("zip -qu -r9 {}.zip * .".format(benchmark), shell=True, cwd=directory) | ||
| benchmark_archive = "{}.zip".format(os.path.join(directory, benchmark)) | ||
| self.logging.info("Created {} archive".format(benchmark_archive)) | ||
|
|
||
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | ||
| mbytes = bytes_size / 1024.0 / 1024.0 | ||
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | ||
|
|
||
| elif language == Language.CPP: | ||
|
|
||
| # lambda C++ runtime build scripts create the .zip file in build directory | ||
| benchmark_archive = os.path.join(directory, "build", "benchmark.zip") | ||
| self.logging.info("Created {} archive".format(benchmark_archive)) | ||
|
|
||
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | ||
| mbytes = bytes_size / 1024.0 / 1024.0 | ||
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | ||
|
|
||
| else: | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Fix incorrect return path for C++ packages.
The packaging logic correctly handles different languages, but there's an issue with the return path calculation for C++ packages.
For C++ (lines 168-177), the code sets benchmark_archive to the build directory path, but the return statement (line 182) still uses the original directory path format, which will be incorrect for C++ packages.
Apply this fix to return the correct archive path for C++:
+ if language == Language.CPP:
+ archive_path = benchmark_archive
+ else:
+ archive_path = os.path.join(directory, "{}.zip".format(benchmark))
+
return (
- os.path.join(directory, "{}.zip".format(benchmark)),
+ archive_path,
bytes_size,
container_uri,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if language in [Language.PYTHON, Language.NODEJS]: | |
| package_config = CONFIG_FILES[language] | |
| function_dir = os.path.join(directory, "function") | |
| os.makedirs(function_dir) | |
| # move all files to 'function' except handler.py | |
| for file in os.listdir(directory): | |
| if file not in package_config: | |
| file = os.path.join(directory, file) | |
| shutil.move(file, function_dir) | |
| # FIXME: use zipfile | |
| # create zip with hidden directory but without parent directory | |
| execute("zip -qu -r9 {}.zip * .".format(benchmark), shell=True, cwd=directory) | |
| benchmark_archive = "{}.zip".format(os.path.join(directory, benchmark)) | |
| self.logging.info("Created {} archive".format(benchmark_archive)) | |
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | |
| mbytes = bytes_size / 1024.0 / 1024.0 | |
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | |
| elif language == Language.CPP: | |
| # lambda C++ runtime build scripts create the .zip file in build directory | |
| benchmark_archive = os.path.join(directory, "build", "benchmark.zip") | |
| self.logging.info("Created {} archive".format(benchmark_archive)) | |
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | |
| mbytes = bytes_size / 1024.0 / 1024.0 | |
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | |
| else: | |
| raise NotImplementedError() | |
| if language in [Language.PYTHON, Language.NODEJS]: | |
| package_config = CONFIG_FILES[language] | |
| function_dir = os.path.join(directory, "function") | |
| os.makedirs(function_dir) | |
| # move all files to 'function' except handler.py | |
| for file in os.listdir(directory): | |
| if file not in package_config: | |
| file = os.path.join(directory, file) | |
| shutil.move(file, function_dir) | |
| # FIXME: use zipfile | |
| # create zip with hidden directory but without parent directory | |
| execute("zip -qu -r9 {}.zip * .".format(benchmark), shell=True, cwd=directory) | |
| benchmark_archive = "{}.zip".format(os.path.join(directory, benchmark)) | |
| self.logging.info("Created {} archive".format(benchmark_archive)) | |
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | |
| mbytes = bytes_size / 1024.0 / 1024.0 | |
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | |
| elif language == Language.CPP: | |
| # lambda C++ runtime build scripts create the .zip file in build directory | |
| benchmark_archive = os.path.join(directory, "build", "benchmark.zip") | |
| self.logging.info("Created {} archive".format(benchmark_archive)) | |
| bytes_size = os.path.getsize(os.path.join(directory, benchmark_archive)) | |
| mbytes = bytes_size / 1024.0 / 1024.0 | |
| self.logging.info("Zip archive size {:2f} MB".format(mbytes)) | |
| else: | |
| raise NotImplementedError() | |
| + # ensure we return the correct archive path for C++ | |
| + if language == Language.CPP: | |
| + archive_path = benchmark_archive | |
| + else: | |
| + archive_path = os.path.join(directory, "{}.zip".format(benchmark)) | |
| + | |
| return ( | |
| - os.path.join(directory, "{}.zip".format(benchmark)), | |
| + archive_path, | |
| bytes_size, | |
| container_uri, | |
| ) |
🤖 Prompt for AI Agents
In sebs/aws/aws.py around lines 148 to 179, the C++ packaging sets
benchmark_archive to the build directory path, but the return statement after
line 179 still uses the original directory path, causing an incorrect return
path for C++ packages. To fix this, update the return statement to use the
benchmark_archive variable as set for C++ so that the correct archive path from
the build directory is returned.
The features that we're planning to implement here are:
Summary by CodeRabbit
New Features
Refactor
Chores