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.

Sign up for GitHub

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

Jump to bottom

Foreign JSON members roundtrip #669

Merged
merged 11 commits into from
May 6, 2022
Merged

Foreign JSON members roundtrip #669

merged 11 commits into from
May 6, 2022

Conversation

Udumft
Copy link
Contributor

@Udumft Udumft commented Mar 25, 2022

Resolves #637
This PR utilizes Turf.ForeignMemberContainer and it's analog for reference types: ForeignMemberClassContainer.
Revisiting types, related to processing various responses from the Directions API, found and refactored some outdated coding logic.

@Udumft Udumft added Core feature New feature request. labels Mar 25, 2022
@Udumft Udumft self-assigned this Mar 25, 2022
Udumft
Udumft commented Mar 25, 2022
View reviewed changes
Tests/MapboxDirectionsTests/RouteStepTests.swift

// https://github.com/mapbox/mapbox-directions-swift/issues/125
var referenceStepJSON = stepJSON
referenceStepJSON.removeValue(forKey: "weight")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now covered by the new feature!

Copy link
Contributor

Choose a reason for hiding this comment

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

#125 still tracks formal support for this property, but it’s great that we can at least allow developers to access the data.

@Udumft Udumft force-pushed the vk/637-json-roundtrip branch from 148e227 to f4d02db Compare April 5, 2022 14:21
@Udumft Udumft requested review from 1ec5 and a team April 5, 2022 14:47
@Udumft Udumft marked this pull request as ready for review April 5, 2022 14:48
This was referenced Apr 6, 2022
Closed
Closed
1ec5
1ec5 approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

This looks good to me, other than probably needing to bring back the ref-in-name code for now. The performance issue is significant but scoped such that we can treat it as tail work if necessary.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/MapboxDirections/RouteResponse.swift Show resolved Hide resolved
Sources/MapboxDirections/RouteStep.swift Show resolved Hide resolved
Sources/MapboxDirections/RouteStep.swift Show resolved Hide resolved
Sources/MapboxDirections/RouteStep.swift Show resolved Hide resolved
@Udumft
Copy link
Contributor Author

Udumft commented Apr 11, 2022

I've tried disabling foreign members coding in Turf to measure performance hit (using this mechanism) but it still slowed down the coding significantly. Probably need to find another approach...

@Udumft
Copy link
Contributor Author

Udumft commented Apr 12, 2022

According to tests metrics, this feature slows down requests decoding up to 3-5 times depending on geometry type. Marking this PR as do not merge until further decision.

Before:
Screenshot 2022-04-12 at 14 08 03
After:
Screenshot 2022-04-12 at 14 08 35

@1ec5
Copy link
Contributor

1ec5 commented Apr 14, 2022

Can we narrow the performance penalty down to a particular property or object in the response? For example, what if the response lacks annotations, which already cause the leg to be doubly decoded even before this change?

@Udumft
Copy link
Contributor Author

Udumft commented May 5, 2022

I found that Road object does not need a foreign member container since it basically codes a subset inside a route step. But instead, it did double coding, treating all other members as foreign, thus decresing the performance significantly. After the update, performance hit has smoothed:
image

1ec5
1ec5 reviewed May 5, 2022
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Hopefully eliminating protocol conformance for the union-style redecoding structs will mitigate the performance hit to an acceptable degree.

Sources/MapboxDirections/RouteStep.swift Outdated Show resolved Hide resolved
Sources/MapboxDirections/RouteStep.swift Outdated Show resolved Hide resolved
@Udumft
vk-637-json-roundtrip: removed excessive ForeignMemberContainer confo…
09e6b50 
…rmance from IntersectionShapeIndex and AdministrativeAreaIndex
@Udumft
Copy link
Contributor Author

Udumft commented May 6, 2022

After updating IntersectionShapeIndex and AdministrativeAreaIndex, test performance seems to stabilize:

image

@Udumft Udumft requested a review from a team May 6, 2022 07:42
1ec5
1ec5 approved these changes May 6, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@1ec5 1ec5 merged commit d832ca1 into main May 6, 2022
@1ec5 1ec5 deleted the vk/637-json-roundtrip branch May 6, 2022 20:30
This was referenced May 6, 2022
Merged
Merged
@1ec5 1ec5 mentioned this pull request Jun 21, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ec5 1ec5 1ec5 approved these changes

Assignees

@Udumft Udumft

Labels
Core feature New feature request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Codable types should roundtrip unrecognized JSON properties
2 participants
@Udumft @1ec5