-
Notifications
You must be signed in to change notification settings - Fork 18
fix: Adding a TypeAdapter cache to fix a performance regression with … #212
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,7 +130,6 @@ addopts = [ | |
| "--timeout=30" | ||
| ] | ||
|
|
||
|
|
||
| [tool.coverage.run] | ||
| branch = true | ||
| parallel = true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
|
|
||
| import time | ||
| import cProfile | ||
| import pstats | ||
| import io | ||
| import logging | ||
| import pytest | ||
| from openjd.model import create_job, decode_job_template | ||
|
|
||
| # Configure logging | ||
| logging.basicConfig( | ||
| level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" | ||
| ) | ||
| logger = logging.getLogger("openjd.model.benchmark") | ||
|
|
||
|
|
||
| class TestBenchmarkStepEnvironmentsPerformance: | ||
| """Benchmark class to verify performance with large numbers of step environments.""" | ||
|
|
||
| def test_job_template_with_many_total_step_environments(self): | ||
| """ | ||
| Benchmark that a job template with many total step environments across multiple steps is processed efficiently. | ||
| This test creates steps with many environments each and verifies the processing time. | ||
| """ | ||
| # Create a job template with multiple steps, each with step environments | ||
| num_steps = 100 # Create 100 steps | ||
| num_step_envs_per_step = 200 # 200 environments per step | ||
|
|
||
| logger.info( | ||
| f"CREATING JOB TEMPLATE WITH {num_steps} STEPS AND {num_step_envs_per_step} ENVIRONMENTS PER STEP" | ||
| ) | ||
|
|
||
| steps = [] | ||
| for step_num in range(num_steps): | ||
| steps.append( | ||
| { | ||
| "name": f"TestStep{step_num}", | ||
| "script": { | ||
| "actions": {"onRun": {"command": "echo", "args": [f"Step {step_num}"]}} | ||
| }, | ||
| "stepEnvironments": [ | ||
| {"name": f"stepEnv{step_num}_{i}", "variables": {"key": f"value{i}"}} | ||
| for i in range(num_step_envs_per_step) | ||
| ], | ||
| } | ||
| ) | ||
|
|
||
| job_template_with_many_total_envs = { | ||
| "specificationVersion": "jobtemplate-2023-09", | ||
| "name": "Test Job with Many Total Step Environments", | ||
| "steps": steps, | ||
| } | ||
|
|
||
| logger.info("STARTING JOB TEMPLATE PROCESSING") | ||
|
|
||
| # Set up profiler | ||
| profiler = cProfile.Profile() | ||
| profiler.enable() | ||
|
|
||
| start_time = time.time() | ||
|
|
||
| try: | ||
| # Create a proper JobTemplate object from the dictionary using decode_job_template | ||
| job_template = decode_job_template(template=job_template_with_many_total_envs) | ||
|
|
||
| # Call create_job with the JobTemplate object | ||
| _ = create_job(job_template=job_template, job_parameter_values={}) | ||
|
|
||
| elapsed_time = time.time() - start_time | ||
| logger.info(f"PERFORMANCE RESULT: create_job completed in {elapsed_time:.2f} seconds") | ||
|
|
||
| # Disable profiler and print results | ||
| profiler.disable() | ||
|
|
||
| # Log the top 20 functions by cumulative time | ||
| s = io.StringIO() | ||
| ps = pstats.Stats(profiler, stream=s).sort_stats("cumulative") | ||
| ps.print_stats(20) | ||
| logger.info("TOP 20 FUNCTIONS BY CUMULATIVE TIME:") | ||
| for line in s.getvalue().splitlines(): | ||
| logger.info(line) | ||
|
|
||
| # Log the top 20 functions by total time | ||
| s = io.StringIO() | ||
| ps = pstats.Stats(profiler, stream=s).sort_stats("time") | ||
| ps.print_stats(20) | ||
| logger.info("TOP 20 FUNCTIONS BY TOTAL TIME:") | ||
| for line in s.getvalue().splitlines(): | ||
| logger.info(line) | ||
|
|
||
| # Verify that the operation completed within a reasonable time | ||
| assert ( | ||
| elapsed_time < 10 | ||
| ), f"Operation took {elapsed_time:.2f} seconds, which exceeds the 10 second threshold" | ||
|
Comment on lines
+93
to
+96
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How close are we to this threshold?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's going to depend on the host which is part of why I've relegated it to "benchmark", but I'm running it in about 4 seconds currently |
||
|
|
||
| except Exception as e: | ||
| # Disable profiler in case of exception | ||
| profiler.disable() | ||
|
|
||
| elapsed_time = time.time() - start_time | ||
| logger.error( | ||
| f"ERROR: create_job failed in {elapsed_time:.2f} seconds with error: {str(e)}" | ||
| ) | ||
|
|
||
| # Log profiling information even in case of failure | ||
| s = io.StringIO() | ||
| ps = pstats.Stats(profiler, stream=s).sort_stats("cumulative") | ||
| ps.print_stats(20) | ||
| logger.info("TOP 20 FUNCTIONS BY CUMULATIVE TIME (BEFORE ERROR):") | ||
| for line in s.getvalue().splitlines(): | ||
| logger.info(line) | ||
|
|
||
| pytest.fail(f"create_job failed with error: {str(e)}") | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be running these in GitHub actions to prevent regression before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at that - I'd like to, but it looks like the commands run live in a shared config currently. If there's a simple way to add that I'm happy to do it though!