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

Parse event data without json #1001

Merged

Chaoba merged 4 commits into main from kl-eventdata

Dec 22, 2021

Contributor

Chaoba commented Dec 16, 2021 •

edited by kiryldz

Loading

Summary of changes

This pr removes json in ObservableExtension and parses the event data manually to improve the performance.
Take getMapIdleEventData for example, before the change it will take 3213802ns to parse the data into MapIdleEventData, while with the solution in this pr, it will be 432447ns.

User impact (optional)

Pull request checklist:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
Add example if relevant.
Document any changes to public APIs.
Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Removes json in ObservableExtension and to improve the performance.</changelog>.
If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

Chaoba self-assigned this

Chaoba added the performance ⚡ label

Contributor

kiryldz commented Dec 16, 2021

@Chaoba can you run some tests and record if CPU / mem usage is better?

tobrun reviewed

View reviewed changes

Member

tobrun left a comment

Impressive amount of work!
Do we have any testing in place for this feature already?
If not, can we add?

sdk-base/src/main/java/com/mapbox/maps/extension/observable/model/DataSourceType.kt Show resolved Hide resolved

sdk-base/src/main/java/com/mapbox/maps/extension/observable/model/DataSourceType.kt Show resolved Hide resolved

alexander-kulikovskii reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Outdated Show resolved Hide resolved

Contributor

alexander-kulikovskii commented Dec 17, 2021

btw, @Chaoba do you have any chance to compare your solution with Kotlin serialization?

Member

tobrun commented Dec 17, 2021

btw, @Chaoba do you have any chance to compare your solution with Kotlin serialization?

interested in knowing this but not a requirement to close this one out.
Direct mapping will always be faster as a serialization approach.

Chaoba force-pushed the kl-eventdata branch from ea7761f to 3c96c7b Compare

December 17, 2021 12:03


          Parse event data without json

930ad32

Chaoba force-pushed the kl-eventdata branch from 3c96c7b to 930ad32 Compare

December 17, 2021 12:06

alexander-kulikovskii approved these changes

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Show resolved Hide resolved


          Add missed docs; fix the failed test cases

809e2c3

alexander-kulikovskii reviewed

View reviewed changes

sdk/src/test/java/com/mapbox/maps/extension/observable/ObservableTest.kt Show resolved Hide resolved


          Add tests for nullable properties

605a0e6

Chaoba force-pushed the kl-eventdata branch from 6980d3c to 605a0e6 Compare

December 20, 2021 03:09

Chaoba marked this pull request as ready for review

December 20, 2021 07:00

Chaoba requested a review from a team as a code owner

December 20, 2021 07:00

alexander-kulikovskii reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Outdated Show resolved Hide resolved

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Show resolved Hide resolved

Chaoba force-pushed the kl-eventdata branch from 21d3a2c to 477b64c Compare

December 20, 2021 10:38

alexander-kulikovskii reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Outdated Show resolved Hide resolved

Chaoba requested review from tobrun and alexander-kulikovskii

December 20, 2021 11:30

yunikkk reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Outdated Show resolved Hide resolved

yunikkk reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/model/RequestType.kt Show resolved Hide resolved

Chaoba requested a review from yunikkk

December 21, 2021 01:54

tobrun reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt Outdated Show resolved Hide resolved

Chaoba force-pushed the kl-eventdata branch from 477b64c to 6493c43 Compare

December 21, 2021 12:07

Chaoba requested a review from tobrun

December 21, 2021 12:08

yunikkk reviewed

View reviewed changes

sdk-base/src/main/java/com/mapbox/maps/extension/observable/ObservableExtension.kt


		internal fun Map<String, Value>.nonNullStringWithUpperCase(name: String): String {
		return nonNullString(name).toUpperCase(Locale.US).replace(DASH, UNDERLINE)

Contributor

yunikkk Dec 21, 2021

What bothers me is that method is named upperCase but it also replaces dash with underline aside from uppercasing the letters. Maybe rename it if not split to two separate methods?

Contributor Author

Chaoba Dec 22, 2021

Right, this method actually gets and transforms the string to a valid enum value, which can be used to create an enum instance.
Change the method name to validEnumValue

yunikkk approved these changes

View reviewed changes


          Add map.nonNullStringWithUpperCase function

645f74e

Chaoba force-pushed the kl-eventdata branch from 6493c43 to 645f74e Compare

December 22, 2021 03:13

tobrun approved these changes

View reviewed changes

Chaoba merged commit ec63e1b into main

Chaoba deleted the kl-eventdata branch

December 22, 2021 09:31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance ⚡