Skip to content

Speed up validator build by avoiding is_file()#23

Merged
osteel merged 3 commits intoosteel:mainfrom
paul-m:avoid-is-file
Feb 1, 2023
Merged

Speed up validator build by avoiding is_file()#23
osteel merged 3 commits intoosteel:mainfrom
paul-m:avoid-is-file

Conversation

@paul-m
Copy link
Contributor

@paul-m paul-m commented Jan 31, 2023

Description

Adds fromYamlRaw() and fromJsonRaw() methods to ValidatorBuilderInterface.

Motivation and context

Our project required testing output of hundreds of permutations of API requests. We noticed that our tests seemed rather slow, so we did some digging and found that ValidatorBuilder calls is_file() for all builds.

Adding a fromYamlRaw() method will allow us to bypass this call and improve performance.

If it fixes an open issue, please link to the issue here (if you write fixes #num
or closes #num, the issue will be automatically closed when the pull is accepted.)

How has this been tested?

Firstly the PR adds test cases to ValidatorBuilderTest to cover the new lines of code.

Secondly, this PR adds a test called ValidatorBuilderSpeedTest. All it does is build a validator using whichever method is uncommented. It's not a super-great test. :-)

We also add a Composer script that runs this test 10,000 times.

Comparison results look like this:

Using fromYaml() and fromJson():

Time: 00:22.492, Memory: 21.15 MB

OK (20000 tests, 20000 assertions)

Using fromYamlRaw() and fromJsonRaw():

Time: 00:14.518, Memory: 21.15 MB

OK (20000 tests, 20000 assertions)

Screenshots (if appropriate)

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests and code style pass.

  • I have read the CONTRIBUTING document.
  • My pull request addresses exactly one patch/feature.
  • I have created a branch for this patch/feature.
  • Each individual commit in the pull request is meaningful.
  • I have added tests to cover my changes.
  • If my change requires a change to the documentation, I have updated it accordingly.

If you're unsure about any of these, don't hesitate to ask. We're here to help!

@paul-m paul-m marked this pull request as draft January 31, 2023 19:42
Copy link
Owner

@osteel osteel left a comment

Choose a reason for hiding this comment

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

Hey Paul! Thank you for your pull request. I understand where you come from with this and any performance gain is nice, although in this case I'd say it's fairly small, all things considered. Does it make a huge difference in your specific use case?

The "issue" I've got with this at the moment is that if we introduce public methods to load string definitions directly, I feel like there should also be methods to load files straight away.

So I'd add fromJsonFile and fromYamlFile as well.

More coherent this way I think, and we'd still not be introducing any breaking changes.

What do you think?

This would also imply restructuring the README a little bit, but you can leave that to me (I already wanted to introduce some cosmetic changes anyway, so could do that at the same time)

@paul-m paul-m marked this pull request as ready for review February 1, 2023 17:43
@osteel osteel merged commit 718cd0c into osteel:main Feb 1, 2023
@osteel
Copy link
Owner

osteel commented Feb 1, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments