Speed up SanitizingJSONEncoder on large objects w/ tests#149
Merged
Conversation
Fixes an issue where trying to attach a large payload to a notification would take a long time. Changes the SanitizingJSONEncoder to use set instead of list for faster lookup of ignored objects.
This tests that we are able to encode objects in a speedy manner
kattrali
previously approved these changes
Apr 5, 2018
Contributor
kattrali
left a comment
kattrali
left a comment
There was a problem hiding this comment.
Looks good, I'd make on tweak to the perf test for easier debugging:
| # json encoding is very slow on python 2.6 so we need to increase | ||
| # the allowable time when running on it | ||
| maximum_time = 18 | ||
| self.assertTrue(time < maximum_time) |
Contributor
There was a problem hiding this comment.
I'd alter the error message to print the values here:
- self.assertTrue(time < maximum_time)
+ self.assertTrue(time < maximum_time,
+ "Encoding required {}s (expected {}s)".format(
+ time, maximum_time
+ ))
kattrali
approved these changes
Apr 6, 2018
Cawllec
reviewed
Apr 6, 2018
| # json encoding is very slow on python 2.6 so we need to increase | ||
| # the allowable time when running on it | ||
| maximum_time = 18 | ||
| self.assertTrue(time < maximum_time, |
Contributor
There was a problem hiding this comment.
I don't like this as a pass/fail metric for tests, as it's too dependent on the performance of the CI tools.
Contributor
There was a problem hiding this comment.
Its dependent on the tools, but also a small canary if something goes terribly wrong under the lax requirements of this test. I'd prefer that we have something for now and look into something more thorough in the future, like a formal perf suite.
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
4 participants
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.
Built on the great work of @jonls.
This adds some tests and makes sure that the methods are backwards compatible.