Skip to content
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.

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

Java benchmarks support #223

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions benchmarks/600.java/601.hello-world/config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"timeout": 120,
"memory": 512,
"languages": ["java"]
}

38 changes: 38 additions & 0 deletions benchmarks/600.java/601.hello-world/java/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd">

<modelVersion>4.0.0</modelVersion>

<!-- Group ID and Artifact ID uniquely identify the project -->
<groupId>faas</groupId>
<artifactId>601.hello-world</artifactId>
<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging> <!-- The packaging type for the project (jar by default) -->

<!-- Configure the Java version (set to Java 8) -->
<properties>
<maven.compiler.source>1.8</maven.compiler.source> <!-- Source compatibility (Java 8) -->
<maven.compiler.target>1.8</maven.compiler.target> <!-- Target compatibility (Java 8) -->
</properties>
Comment on lines +14 to +18
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider updating to a more recent Java version.

The project is currently configured to use Java 8 (1.8) for both source and target compatibility. While Java 8 is still widely used, it's becoming outdated. Consider updating to a more recent LTS version like Java 11 or 17 to benefit from performance improvements and newer language features.

To update, you would change these lines:

<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>

Or for Java 17:

<maven.compiler.source>17</maven.compiler.source>
<maven.compiler.target>17</maven.compiler.target>


<!-- Define the main class to run the JAR -->
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.2.0</version>
<configuration>
<archive>
<manifest>
<mainClass>faas.App</mainClass> <!-- Replace with your main class -->
</manifest>
</archive>
</configuration>
</plugin>
</plugins>
</build>
</project>
Comment on lines +20 to +37
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Maven-jar-plugin version is outdated and dependencies are missing.

  • The maven-jar-plugin version is 3.2.0. Update to version 3.3.0 for potential bug fixes and improvements:

    <version>3.3.0</version>
  • The <dependencies> section is missing in the pom.xml. Ensure to include all necessary dependencies to avoid runtime issues.

🔗 Analysis chain

Update plugin version and review main class package name.

  1. The maven-jar-plugin version (3.2.0) is not the latest. Consider updating to the most recent version (currently 3.3.0) for potential bug fixes and improvements:
<version>3.3.0</version>
  1. The main class is set to "faas.App". The package name "faas" is unconventional and doesn't align with standard Java package naming conventions. Consider using a more descriptive and hierarchical package name, e.g., "com.spcl.serverlessbenchmarks.helloworld.App".

  2. There are no dependencies defined in this pom.xml. If your project requires any external libraries, make sure to add them in a <dependencies> section.

To verify the main class exists and follows the package structure, run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the main class file exists
fd --type f --full-path '.*src/main/java/faas/App.java$'

# Check the package declaration in the main class file
rg --type java '^package faas;' src/main/java/faas/App.java

Length of output: 257


Script:

#!/bin/bash
# Verify the package declaration in the main class file
rg --type java '^package faas;' benchmarks/600.java/601.hello-world/java/src/main/java/faas/App.java

Length of output: 116


Script:

#!/bin/bash
# Extract the version of maven-jar-plugin
rg '<artifactId>maven-jar-plugin<\/artifactId>' -A1 benchmarks/600.java/601.hello-world/java/pom.xml | rg '<version>([^<]+)<\/version>' -o

# Check for the presence of a <dependencies> section
rg '<dependencies>' benchmarks/600.java/601.hello-world/java/pom.xml

Length of output: 460


Script:

#!/bin/bash
# Extract the version of maven-jar-plugin
rg '<artifactId>maven-jar-plugin</artifactId>' -A1 benchmarks/600.java/601.hello-world/java/pom.xml | rg '<version>([^<]+)</version>' -o

# Check for the presence of a <dependencies> section
rg '<dependencies>' benchmarks/600.java/601.hello-world/java/pom.xml

Length of output: 234


Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package faas;

public class App {
public static void main(String[] args) {
System.out.println("Hellooooooooooooooooooo, World!");
}
Comment on lines +4 to +6
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refine the output message and consider adding benchmark-specific functionality.

  1. The current output "Hellooooooooooooooooooo, World!" seems unusual for a standard "Hello, World!" program. Consider using a more conventional "Hello, World!" message unless there's a specific reason for the extended greeting.

  2. For benchmarking purposes, consider adding more functionality to this method. Some suggestions:

    • Add a parameter to control the number of times the message is printed.
    • Include a simple computation to measure performance.
    • Add timing code to measure execution time.
    • Implement logging to capture benchmark results.

Here's an example of how you might modify the method:

public static void main(String[] args) {
    int iterations = args.length > 0 ? Integer.parseInt(args[0]) : 1;
    long startTime = System.nanoTime();
    
    for (int i = 0; i < iterations; i++) {
        System.out.println("Hello, World!");
    }
    
    long endTime = System.nanoTime();
    long duration = (endTime - startTime) / 1_000_000;  // Convert to milliseconds
    System.out.printf("Executed %d iterations in %d ms%n", iterations, duration);
}

This modification allows for multiple iterations and includes basic timing, which could be useful for benchmarking purposes.

}
55 changes: 55 additions & 0 deletions benchmarks/wrappers/openwhisk/java/Main.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import com.google.gson.JsonObject;
import com.example.project.App ;
import java.time.Instant;
import java.time.Duration;
import java.io.File;
import java.io.IOException;


public class Main {
public static JsonObject main(JsonObject args) {

// Logger logger = Logger.getLogger(FunctionHandler.class.getName());
// logger.setLevel(Level.INFO);

Gson gson = new Gson();
App function = new App();

Instant begin = Instant.now();
JsonObject result = function.handler(args);
Instant end = Instant.now();

long computeTime = Duration.between(begin, end).toNanos() / 1000; // Convert nanoseconds to microseconds
Comment on lines +18 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for App.handler call

The App.handler method is called without any error handling. If an exception occurs during the execution of this method, it will propagate and potentially crash the application.

Consider wrapping the App.handler call in a try-catch block to handle potential exceptions gracefully. For example:

JsonObject result;
try {
    result = function.handler(args);
} catch (Exception e) {
    // Log the error
    result = new JsonObject();
    result.addProperty("error", "An error occurred while processing the request: " + e.getMessage());
}

This will ensure that even if an error occurs, the function will return a meaningful response instead of crashing.


boolean isCold = false;
String fileName = "/cold_run";

File file = new File(fileName);
if (!file.exists()) {
isCold = true;
try {
file.createNewFile();
} catch (IOException e) {
e.printStackTrace();
}
}
Comment on lines +24 to +35
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve cold start detection mechanism

The current implementation of cold start detection has a few potential issues:

  1. The file path "/cold_run" might not be writable in all environments.
  2. The error handling for file creation only prints the stack trace, which might not be visible in some serverless environments.
  3. This approach may not accurately detect cold starts in all scenarios, especially if the container is reused but the file is deleted.

Consider the following improvements:

  1. Use a more reliable method for cold start detection, such as caching a static variable or using environment variables specific to your serverless platform.
  2. If you must use a file-based approach, use a path that's guaranteed to be writable, such as /tmp/cold_run.
  3. Improve error handling to log errors properly and potentially set isCold to true if file creation fails.

Here's an example of how you might refactor this:

private static boolean isFirstExecution = true;

// In the main method:
boolean isCold = isFirstExecution;
if (isFirstExecution) {
    isFirstExecution = false;
}

This approach uses a static variable to detect the first execution, which is typically equivalent to a cold start in serverless environments.


// Convert to Unix timestamp in seconds.microseconds
String formattedBegin = String.format("%d.%06d", begin.getEpochSecond(), begin.getNano() / 1000); // Convert nanoseconds to microseconds
String formattedEnd = String.format("%d.%06d", end.getEpochSecond(), end.getNano() / 1000);

String requestId = System.getenv("__OW_ACTIVATION_ID");

JsonObject jsonResult = new JsonObject();
jsonObject.put("begin", formattedBegin);
jsonObject.put("end", formattedEnd);
jsonObject.put("request_id", "requestId");
jsonObject.put("compute_time", computeTime);
jsonObject.put("is_cold", isCold);
jsonObject.put("result", result);
return jsonResult;
}
Comment on lines +37 to +51
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typos and correct JsonObject usage

There are a few issues in the result formatting section:

  1. There's a typo in the variable name when adding the request ID to the result.
  2. The method for adding properties to the JsonObject is incorrect. Java's JsonObject uses addProperty instead of put.

Apply the following changes to fix these issues:

-        JsonObject jsonResult = new JsonObject();
-        jsonObject.put("begin", formattedBegin); 
-        jsonObject.put("end", formattedEnd);
-        jsonObject.put("request_id", "requestId");  
-        jsonObject.put("compute_time", computeTime);
-        jsonObject.put("is_cold", isCold);
-        jsonObject.put("result", result);
+        JsonObject jsonResult = new JsonObject();
+        jsonResult.addProperty("begin", formattedBegin);
+        jsonResult.addProperty("end", formattedEnd);
+        jsonResult.addProperty("request_id", requestId);
+        jsonResult.addProperty("compute_time", computeTime);
+        jsonResult.addProperty("is_cold", isCold);
+        jsonResult.add("result", result);
         return jsonResult;

Note that for the "result" field, we use add instead of addProperty because result is itself a JsonObject.

📝 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.

Suggested change
// Convert to Unix timestamp in seconds.microseconds
String formattedBegin = String.format("%d.%06d", begin.getEpochSecond(), begin.getNano() / 1000); // Convert nanoseconds to microseconds
String formattedEnd = String.format("%d.%06d", end.getEpochSecond(), end.getNano() / 1000);
String requestId = System.getenv("__OW_ACTIVATION_ID");
JsonObject jsonResult = new JsonObject();
jsonObject.put("begin", formattedBegin);
jsonObject.put("end", formattedEnd);
jsonObject.put("request_id", "requestId");
jsonObject.put("compute_time", computeTime);
jsonObject.put("is_cold", isCold);
jsonObject.put("result", result);
return jsonResult;
}
// Convert to Unix timestamp in seconds.microseconds
String formattedBegin = String.format("%d.%06d", begin.getEpochSecond(), begin.getNano() / 1000); // Convert nanoseconds to microseconds
String formattedEnd = String.format("%d.%06d", end.getEpochSecond(), end.getNano() / 1000);
String requestId = System.getenv("__OW_ACTIVATION_ID");
JsonObject jsonResult = new JsonObject();
jsonResult.addProperty("begin", formattedBegin);
jsonResult.addProperty("end", formattedEnd);
jsonResult.addProperty("request_id", requestId);
jsonResult.addProperty("compute_time", computeTime);
jsonResult.addProperty("is_cold", isCold);
jsonResult.add("result", result);
return jsonResult;
}

}



Empty file.
69 changes: 69 additions & 0 deletions config/example2.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
{
"experiments": {
"deployment": "openwhisk",
"update_code": false,
"update_storage": false,
"download_results": false,
"runtime": {
"language": "java",
"version": "8"
},
"type": "invocation-overhead",
"perf-cost": {
"benchmark": "601.hello-world",
"experiments": ["cold", "warm", "burst", "sequential"],
"input-size": "test",
"repetitions": 50,
"concurrent-invocations": 50,
"memory-sizes": [128, 256]
},
"network-ping-pong": {
"invocations": 50,
"repetitions": 1000,
"threads": 1
},
"invocation-overhead": {
"repetitions": 5,
"N": 20,
"type": "payload",
"payload_begin": 1024,
"payload_end": 6251000,
"payload_points": 20,
"code_begin": 1048576,
"code_end": 261619712,
"code_points": 20
},
"eviction-model": {
"invocations": 1,
"function_copy_idx": 0,
"repetitions": 5,
"sleep": 1
}
},
"deployment": {
"openwhisk": {
"shutdownStorage": false,
"removeCluster": false,
"wskBypassSecurity": "true",
"wskExec": "wsk",
"experimentalManifest": false,
"docker_registry": {
"registry": "",
"username": "",
"password": ""
},
"storage": {
"address": "",
"mapped_port": 9011,
"access_key": "",
"secret_key": "",
"instance_id": "",
"output_buckets": [],
"input_buckets": [],
"type": "minio"
}

}
}
}
Comment on lines +43 to +68
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review security settings and sensitive information handling

The deployment configuration for OpenWhisk is comprehensive. However, there are some security considerations to address:

  1. The wskBypassSecurity is set to "true". This could pose a security risk if used in a production environment.
  2. Sensitive information fields (Docker registry credentials, storage access keys) are left empty. Ensure you have a secure method to populate these fields in different environments.

Consider the following improvements:

  1. Set wskBypassSecurity to false unless absolutely necessary for testing purposes.
  2. Implement a secure method to inject sensitive information (e.g., environment variables, secrets management system) rather than hardcoding them in the configuration file.
  3. Add a comment in the file to indicate how sensitive information should be handled.

Example:

"wskBypassSecurity": false,
// ... other fields ...
"docker_registry": {
  "registry": "", // Set via DOCKER_REGISTRY env var
  "username": "", // Set via DOCKER_USERNAME env var
  "password": ""  // Set via DOCKER_PASSWORD env var
},


18 changes: 18 additions & 0 deletions config/systems.json
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,24 @@
"minio": "7.0.16"
}
}
},
"java": {
"base_images": {
"8": "openwhisk/java8action"
},
"images": [
"function"
],
"username": "docker_user",
"deployment": {
"files": [
"Main.java",
"Storage.java"
],
"packages": {
"minio": "8.5.9"
}
}
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions dockerfiles/openwhisk/java/Dockerfile.function
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
ARG BASE_IMAGE
FROM $BASE_IMAGE
COPY . /function/

RUN apt-get update && apt-get install -y maven
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize Maven installation to reduce image size

The Maven installation step is correct, but it can be optimized to reduce the final image size.

Consider updating the Maven installation command to clean up the apt cache:

RUN apt-get update && \
    apt-get install -y maven && \
    apt-get clean && \
    rm -rf /var/lib/apt/lists/*

This will remove unnecessary package files after installation, resulting in a smaller image size.


# Check if pom.xml exists before running Maven
RUN if [ -f ./pom.xml ]; then mvn clean install; else echo "pom.xml not found, aborting build." && exit 1; fi
2 changes: 1 addition & 1 deletion sebs.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def simplified_common_params(func):
@click.option(
"--language",
default=None,
type=click.Choice(["python", "nodejs"]),
type=click.Choice(["python", "nodejs", "java"]),
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Inconsistencies: Missing "java" in Other Language Options

Several files still list only "python" and "nodejs" as language options. Please update the following files to include "java":

  • tools/build_docker_images.py
  • scripts/run_experiments.py
  • tests/aws/create_function.py
  • dockerfiles/local/runner.py
🔗 Analysis chain

LGTM: Java language support added

The change adds "java" as an option for the --language parameter, which is consistent with the PR objective of adding Java support to the SeBS framework.

To ensure this change is properly integrated, let's verify if there are any other parts of the code that might need updates to support Java:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of language-related code
rg --type python 'language.*python.*nodejs' sebs.py
rg --type python 'language.*python.*nodejs' .

Length of output: 163


Script:

#!/bin/bash
# Search for other occurrences of language-related code with correct file type
rg --type py 'language.*python.*nodejs' sebs.py
rg --type py 'language.*python.*nodejs' .

Length of output: 1219

help="Benchmark language",
)
@click.option("--language-version", default=None, type=str, help="Benchmark language version")
Expand Down
25 changes: 25 additions & 0 deletions sebs/benchmark.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,11 +250,24 @@ def copy_code(self, output_dir):
FILES = {
"python": ["*.py", "requirements.txt*"],
"nodejs": ["*.js", "package.json"],
"java": ["pom.xml"],
}
path = os.path.join(self.benchmark_path, self.language_name)

for file_type in FILES[self.language_name]:
for f in glob.glob(os.path.join(path, file_type)):
shutil.copy2(os.path.join(path, f), output_dir)

# copy src folder of java (java benchmarks are maven project and need directories)
if self.language_name == "java":
output_src_dir = os.path.join(output_dir, "src")

if os.path.exists(output_src_dir):
# If src dir in output exist, remove the directory and all its contents
shutil.rmtree(output_src_dir)
#To have contents of src directory in the direcory named src located in output
shutil.copytree(os.path.join(path, "src"), output_src_dir)

Comment on lines +261 to +270
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve indentation, comments, and error handling in the Java source copying logic

The indentation of comments is inconsistent, which can affect readability. Additionally, it's advisable to check if the source directory exists before attempting to copy it, to avoid potential exceptions if the directory doesn't exist.

Apply this diff to correct the issues:

    # copy src folder of java (Java benchmarks are Maven projects and need directories)
    if self.language_name == "java":
        output_src_dir = os.path.join(output_dir, "src")
        # If src dir in output exists, remove the directory and all its contents
        if os.path.exists(output_src_dir):
            shutil.rmtree(output_src_dir)
        src_dir = os.path.join(path, "src")
        # Check if src directory exists in the benchmark path
+       if os.path.exists(src_dir):
            shutil.copytree(src_dir, output_src_dir)
📝 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.

Suggested change
# copy src folder of java (java benchmarks are maven project and need directories)
if self.language_name == "java":
output_src_dir = os.path.join(output_dir, "src")
if os.path.exists(output_src_dir):
# If src dir in output exist, remove the directory and all its contents
shutil.rmtree(output_src_dir)
#To have contents of src directory in the direcory named src located in output
shutil.copytree(os.path.join(path, "src"), output_src_dir)
# copy src folder of java (Java benchmarks are Maven projects and need directories)
if self.language_name == "java":
output_src_dir = os.path.join(output_dir, "src")
# If src dir in output exists, remove the directory and all its contents
if os.path.exists(output_src_dir):
shutil.rmtree(output_src_dir)
src_dir = os.path.join(path, "src")
# Check if src directory exists in the benchmark path
if os.path.exists(src_dir):
shutil.copytree(src_dir, output_src_dir)

# support node.js benchmarks with language specific packages
nodejs_package_json = os.path.join(path, f"package.json.{self.language_version}")
if os.path.exists(nodejs_package_json):
Expand Down Expand Up @@ -288,6 +301,16 @@ def add_deployment_files(self, output_dir):
for file in handlers:
shutil.copy2(file, os.path.join(output_dir))

def add_deployment_package_java(self, output_dir):
# append to the end of requirements file
packages = self._system_config.deployment_packages(
self._deployment_name, self.language_name
)
if len(packages):
with open(os.path.join(output_dir, "requirements.txt"), "a") as out:
for package in packages:
out.write(package)

Comment on lines +304 to +313
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect dependency handling: Appending to 'requirements.txt' in Java project

The add_deployment_package_java method appends Java packages to requirements.txt, which is used for Python dependencies. In Java Maven projects, dependencies should be managed through the pom.xml file, not requirements.txt.

Consider modifying the method to add the deployment packages to the pom.xml file by updating the ``` section accordingly.

def add_deployment_package_python(self, output_dir):
# append to the end of requirements file
packages = self._system_config.deployment_packages(
Expand Down Expand Up @@ -319,6 +342,8 @@ def add_deployment_package(self, output_dir):
self.add_deployment_package_python(output_dir)
elif self.language == Language.NODEJS:
self.add_deployment_package_nodejs(output_dir)
elif self.language == Language.JAVA:
self.add_deployment_package_java(output_dir)
else:
raise NotImplementedError

Expand Down
3 changes: 2 additions & 1 deletion sebs/faas/function.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ def deserialize(cached_config: dict) -> "Trigger":
class Language(Enum):
PYTHON = "python"
NODEJS = "nodejs"
JAVA = "java"

# FIXME: 3.7+ python with future annotations
@staticmethod
Expand Down Expand Up @@ -299,7 +300,7 @@ def serialize(self) -> dict:

@staticmethod
def deserialize(config: dict) -> Runtime:
languages = {"python": Language.PYTHON, "nodejs": Language.NODEJS}
languages = {"python": Language.PYTHON, "nodejs": Language.NODEJS, "java": Language.JAVA}
return Runtime(language=languages[config["language"]], version=config["version"])


Expand Down
3 changes: 2 additions & 1 deletion sebs/openwhisk/openwhisk.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ def build_base_image(
)

for fn in os.listdir(directory):
if fn not in ("index.js", "__main__.py"):
if fn not in ("index.js", "__main__.py", "Main.java"):
file = os.path.join(directory, fn)
shutil.move(file, build_dir)

Expand Down Expand Up @@ -219,6 +219,7 @@ def package_code(
CONFIG_FILES = {
"python": ["__main__.py"],
"nodejs": ["index.js"],
"java": ["Main.java"],
}
package_config = CONFIG_FILES[language_name]

Expand Down