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

Keyed/unkeyed boxes as structs #36

Merged
merged 7 commits into from Jan 13, 2019
Merged

Keyed/unkeyed boxes as structs #36

merged 7 commits into from Jan 13, 2019

Conversation

regexident
Copy link
Contributor

Not sure yet, if this will actually lead to an improvement of performance.

Running the benchmark from #34 should give a better image of the situation.

🏗Consider this an experimental WIP.

@codecov
Copy link

codecov bot commented Dec 23, 2018

Codecov Report

Merging #36 into master will increase coverage by 0.14%.
The diff coverage is 73.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   74.51%   74.66%   +0.14%     
==========================================
  Files          30       31       +1     
  Lines        1440     1551     +111     
==========================================
+ Hits         1073     1158      +85     
- Misses        367      393      +26
Impacted Files Coverage Δ
Sources/XMLCoder/Box/KeyedBox.swift 100% <ø> (ø) ⬆️
Sources/XMLCoder/Decoder/XMLDecodingStorage.swift 81.25% <100%> (+8.52%) ⬆️
Sources/XMLCoder/Box/SharedBox.swift 100% <100%> (ø)
Sources/XMLCoder/Decoder/XMLDecoder.swift 70.04% <100%> (ø) ⬆️
Sources/XMLCoder/Box/UnkeyedBox.swift 100% <100%> (ø) ⬆️
...XMLCoder/Encoder/XMLUnkeyedEncodingContainer.swift 13.75% <26.66%> (+1.75%) ⬆️
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift 59.78% <29.41%> (-1.67%) ⬇️
...XMLCoder/Decoder/XMLUnkeyedDecodingContainer.swift 25.53% <30%> (+1.1%) ⬆️
...urces/XMLCoder/Encoder/XMLReferencingEncoder.swift 58.97% <37.5%> (+2.21%) ⬆️
Sources/XMLCoder/Encoder/XMLEncoder.swift 81.05% <66.66%> (ø) ⬆️
... and 4 more

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 43aa825...fbebe5f. Read the comment docs.

@regexident
Copy link
Contributor Author

Upon further inspection (ie. running #34 repeatedly) the performance seems to have remained stable.

I'd thus propose to merge this PR as it unifies the representation of T where T: Box and makes it explicit where shared reference semantics is expected.

@regexident regexident changed the title [PoC] A proof-of-concept of keyed/unkeyed boxes as structs Keyed/unkeyed boxes as structs Jan 2, 2019
@regexident
Copy link
Contributor Author

Any chance to have this merged, @MaxDesiatov/@hodovani?

@MaxDesiatov
Copy link
Collaborator

@regexident I've fixed the conflicts and there's still a superEncoder test that fails here. But the most interesting part was that the performance is consistently 10-30% worse, could this be caused by copies of value types? Should we postpone this PR until Swift 5.0 is released or whatever the version is that allows us to improve performance of value types?

screen shot 2019-01-10 at 12 13 14

@MaxDesiatov
Copy link
Collaborator

All tests are fixed now and since this is a dependency of #55, I'm inclined to merge both of these. After reviewing related issues I'd prefer XMLCoder to be slightly slower, but with #12, #17 and #25 fixed than the opposite 😞

@MaxDesiatov MaxDesiatov merged commit 85e977c into CoreOffice:master Jan 13, 2019
@regexident regexident deleted the fix/class-to-struct branch January 13, 2019 15:45
@regexident
Copy link
Contributor Author

Thanks @MaxDesiatov!

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.

None yet

3 participants