Skip to content
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

Creating Bytes from JSONValue with a Pointer #2

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabianfett
Copy link
Collaborator

@fabianfett fabianfett commented Feb 21, 2020

This is an idea on improving the encoding speed. Doesn't seem to work though.

@fabianfett
Copy link
Collaborator Author

fabianfett commented Feb 21, 2020

Codecov Report

Merging #2 into master will increase coverage by 1.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #2      +/-   ##
==========================================
+ Coverage   70.16%   71.37%   +1.21%     
==========================================
  Files          22       23       +1     
  Lines        1790     2264     +474     
==========================================
+ Hits         1256     1616     +360     
- Misses        534      648     +114     
Impacted Files Coverage Δ
Tests/JSONParsingTests/StringParserTests.swift 90.71% <0.00%> (-9.29%) ⬇️
Sources/PureSwiftJSONParsing/JSONValue.swift 48.96% <0.00%> (-42.38%) ⬇️
Tests/JSONParsingTests/BoolParserTests.swift 94.68% <0.00%> (-2.05%) ⬇️
Tests/JSONParsingTests/NullParserTests.swift 93.75% <0.00%> (-1.49%) ⬇️
...ingTests/Learning/FondationJSONSerialization.swift 72.22% <0.00%> (ø)
Sources/PureSwiftJSONParsing/JSONParser.swift 92.94% <0.00%> (+1.70%) ⬆️
Sources/PureSwiftJSONParsing/DocumentReader.swift 94.08% <0.00%> (+6.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59d69a7...a48acbe. Read the comment docs.

@fabianfett fabianfett force-pushed the JSONValue-to-Bytes-using-Pointer branch from ea7aa98 to a48acbe Compare February 29, 2020 18:01
self.write(byte: UInt8(ascii: "e"))

case .bool(false):
self.write(byte: UInt8(ascii: "f"))
Copy link

Choose a reason for hiding this comment

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

These should be in bulk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The implementation I use currently looks like this? Do you think this would be faster in this case?

    case .bool(true):
      bytes.append(contentsOf: [
        UInt8(ascii: "t"), UInt8(ascii: "r"), UInt8(ascii: "u"), UInt8(ascii: "e")
      ])

}
self.write(byte: UInt8(ascii: "\""))
case .number(let string):
string.utf8.withContiguousStorageIfAvailable {
Copy link

Choose a reason for hiding this comment

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

What if contiguous storage isn't available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just a proof of concept, if I can make it faster with a pointer. So far I can't which is why I don't support nsstring for now. ;)

}
case .array(let array):
var iterator = array.makeIterator()
self.write(byte: UInt8(ascii: "["))
Copy link

Choose a reason for hiding this comment

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

Iterators copy values more often than needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Am I correct in assuming that you would vote for direct array access? for elem in array?

@fabianfett
Copy link
Collaborator Author

@Joannis thanks for your guidance.

@fabianfett fabianfett changed the base branch from master to main October 9, 2020 12:52
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