Only store unwrapped content #2
Merged
Conversation
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
florianrhein
suggested changes
Jul 26, 2022
| @@ -139,9 +150,21 @@ public struct JSON { | |||
| if let data = jsonString.data(using: .utf8) { | |||
| self.init(data) | |||
There was a problem hiding this comment.
Suggested change
| self.init(data) | |
| self.init(data) |
I know we should stick to the style of the author, but this indentation is most likely accidentally, as other ifs are also indented with four spaces
florianrhein
approved these changes
Jul 26, 2022
svenmuennich
added a commit
that referenced
this pull request
Jul 28, 2022
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 improves how
JSONhandles its content. Previously the content was unwrapped when initializing a newJSONvalue. This was done to support nesting values of typeJSONinto e.g. dictionaries. However, when unwrapping a nestedJSONvalue its underlying raw value (i.e.object) was extracted, loosing all information previously gather about its content. This approach only improved performance when working with the raw underlying values ofJSON, e.g. when usingobject,arrayObjectordictionaryObject, which should be the exception.With this PR, the
Contentis strongly typed to only store raw primitives or array/dictionaries ofContentvalues. This requires resolving the whole JSON upon initializing a newJSONvalue once. All subsequent accesses to any of its values (e.g. via subscript) are super cheap now, as we can just re-use the previously extracted content value. Resolving the JSON upon initialization is also cheaper now when working with nestedJSONvalues, as we can just re-use their content as well.To see the performance improvements this PR brings you can copy the new performance test
testLongNestedArrayPerformanceto a checkout ofmasterto get a baseline timing value for comparison.Note on the code style: I tried to stay as close as possible to the code style of the upstream repository and only apply minimal fixes.