perf!: testing yaml optimisations#2361
Draft
james-garner-canonical wants to merge 16 commits intocanonical:mainfrom
Draft
perf!: testing yaml optimisations#2361james-garner-canonical wants to merge 16 commits intocanonical:mainfrom
james-garner-canonical wants to merge 16 commits intocanonical:mainfrom
Conversation
Collaborator
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
I like most of these changes - need to review the performance numbers a bit. I'm a bit concerned about charms not being able to read the YAML files. I assume that's going to come up in the discussion today.
Contributor
|
stand-up comments: a more useful metric would be if a super-tox run is faster and by how much, assuming that the same number of tests a run and pass. |
9 tasks
Merge branch 'main' into 26-03+feat+testing-yaml-optimisations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR takes the most drastic changes possible to address the performance concerns raised in #1498. My thinking is that we can see what this breaks and what the maximum possible performance gains are, and then decide what changes (if any) we should actually make.
Changes
This PR caches the file loads performed by
_CharmSpec.autoload, so we only read from disc once per file per testing process instead of once perContextcreation (e.g. once per test).This PR eliminates these extra file loads entirely by creating the
CharmMetaobject from the already created_CharmSpecobject. We still create the temporary directory, as it's where container writes occur.This PR drops these file writes. It seems they were only used for creating
CharmMetaobjects. In principle, charms could assume that these files are exist and try to read them, but apparently this doesn't come up in any of the tests for charms we check.Running super-tox results in 70 of the 153 charms passing on both
mainand on this branch.Two of our tests that explicitly assert on behaviour changed in this PR changes have been updated.
Benchmarking
Benchmark results look promising. The fast benchmarks don't seem to have changed much, while the slowest one (
test_many_tests_autoload_meta) goes from ~9.87 ms to ~4.53 ms. That said, these numbers indicate that there don't really seem to be major performance gains on the table here.Benchmark results on main
Benchmark results on this branch
Resolves: #1498