Skip to content

Change value converters searching #204

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
Merged
merged 3 commits into from
Apr 28, 2022
Merged

Change value converters searching #204

merged 3 commits into from
Apr 28, 2022

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Apr 14, 2022 •
edited
Loading

  • Added perfomance test based on JMH
  • Refactored converters magic for this purpose (point one) RFC: ValueConverters search optimization #178 (comment) . In short, the converters containing the implementing and ValueConverter and ObjectConveter are divided into separate classes for ease of use and in order to be able to register a valueConverter of this type ImmutableStringValueImpl -> String, and an objectConverter of this type String -> StringValue. More details in the code 😁
  • Also, in order for the names to be consistent, the names of the converter classes were changed
  • And additional package directories are added, because we had more than 50 files in the mappers directory and not all mappers, some converters
  • Removed reflection searching to interface of MsgPack Lib enitities (point two) RFC: ValueConverters search optimization #178 (comment)

Closes #178

@ArtDu ArtDu changed the title WIP: Remove converters' find with reflection WIP: Remove the search for converters with reflection Apr 14, 2022
@vrogach2020
Copy link
Contributor

It would be nice to see some perf tests results here on simple and complex types

@ArtDu ArtDu mentioned this pull request Apr 15, 2022
Merged
@ArtDu ArtDu changed the title WIP: Remove the search for converters with reflection WIP: Remove the search with reflection for converters Apr 15, 2022
@akudiyar
Copy link
Collaborator

@ArtDu please add a brief description of the changes -- what was changed, why this change is needed.

akudiyar
akudiyar requested changes Apr 16, 2022
View reviewed changes
@ArtDu ArtDu force-pushed the perf branch 7 times, most recently from c143ef0 to eec2396 Compare April 19, 2022 17:17
@ArtDu ArtDu changed the title WIP: Remove the search with reflection for converters Change value converters searching Apr 19, 2022
@ArtDu ArtDu marked this pull request as ready for review April 19, 2022 17:24
ArtDu
ArtDu commented Apr 20, 2022
View reviewed changes
src/main/java/io/tarantool/driver/api/MessagePackMapperBuilder.java
Comment on lines +4 to +5
import io.tarantool.driver.mappers.converters.ObjectConverter;
import io.tarantool.driver.mappers.converters.ValueConverter;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind leaving converters in the same folders to avoid breakin changes, but it seems better to separate converters and mappers, given that there are more than 50 files in one package

vrogach2020
vrogach2020 previously approved these changes Apr 22, 2022
View reviewed changes
@ArtDu ArtDu requested a review from akudiyar April 22, 2022 11:11
@ArtDu
Copy link
Contributor Author

ArtDu commented Apr 26, 2022

Should test perf of converting with TarantoolTuple and getting with targetType

TarantoolTuple tuple = ...
tuple.getObject("fieldName", Integer.class)

ArtDu added 3 commits April 27, 2022 18:38
@ArtDu
Add perf tests based on jmh
15a8441
@ArtDu
Refactor converters magic
dc9c5a4 
* The converters containing the implementing and ValueConverter and
 ObjectConveter are divided into separate classes for ease of use and
 in order to be able to register a valueConverter of this type
 ImmutableStringValueImpl -> String, and an objectConverter of this type
 String -> StringValue
* In order for the names to be consistent, the names of the converter
 classes were changed
* Additional package directories are added, because we had more than
 50 files in the mappers directory and not all mappers, some converters.
@ArtDu
Remove the search for converters with reflection
0415bad 
Reflexive lookup for value converters has been replaced with accepting
class implementations, since msgpack.unpacker gives them.

Closes #178
akudiyar
akudiyar approved these changes Apr 28, 2022
View reviewed changes
Copy link
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

LGTM.
However I highly recommend to do at least local testing of the new changes with SpringData before merging.

P.S. The new converters should go in a follow-up PR. The GH review interface is already completely broken at this number of changes +_+

@ArtDu
Copy link
Contributor Author

ArtDu commented Apr 28, 2022

LGTM. However I highly recommend to do at least local testing of the new changes with SpringData before merging.

P.S. The new converters should go in a follow-up PR. The GH review interface is already completely broken at this number of changes +_+

Yeah, I tested snapshot with cartridge-springdata, we have to rewrite registration of TarantoolAutoConverter but everything else looks fine

@ArtDu ArtDu merged commit 4ee162b into master Apr 28, 2022
@wey1and wey1and deleted the perf branch May 19, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akudiyar akudiyar akudiyar approved these changes

+1 more reviewer

@vrogach2020 vrogach2020 vrogach2020 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RFC: ValueConverters search optimization

3 participants

@ArtDu @vrogach2020 @akudiyar