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

Possible performance improvement of constant/defaultValue primitive to String mappings #1401

Closed
twentylemon opened this Issue Mar 19, 2018 · 14 comments

Comments

Projects
None yet
3 participants
@twentylemon
Copy link
Contributor

twentylemon commented Mar 19, 2018

When a constant primitive to string mapping is specified (such as Mapping(constant = "1") or @Mapping(defaultValue = "1")), the code generated is:

bean.setIntAttribute(Integer.parseInt("1"));

But it is possible to instead write this as

bean.setIntAttribute(1);

Since Integer.parseInt("1") is known at compile time. A similar improvement can be made for the reverse mapping, which currently uses String.valueOf(<primitive>) at runtime.

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Mar 24, 2018

MapStruct only knows its a String, since that's what the defaultValue in essence is. It does not do any analysis on the value. Of course this could be done (interpreting that the string is only comprised of digits, or digits with a designator (e.g. L for long). I think the code is more or less the same as for constants Be careful though, we should think what we do if the constant is too large for its target type. Follow the same rules als JDK?

I'm not sure when one of us has time to implement this. But feel free to contribute.

@filiphr

This comment has been minimized.

Copy link
Member

filiphr commented Mar 30, 2018

@twentylemon This can be achieved (partially) with the upcoming 1.3 release and the new defaultValueExpression added in #1372.

You would need to write @Mapping(defaultValueExpression = "java(1)"). Is this sufficient for you?

If a constantExpression is also needed can we rename this issue for that, or close this and open a new one?

In any case as @sjaakd mentioned we won't have time to implement this. However, if someone wants to give it a shot we can help out.

@twentylemon

This comment has been minimized.

Copy link
Contributor Author

twentylemon commented Mar 30, 2018

If a constantExpression is also needed can we rename this issue for that, or close this and open a new one?

Does defaultValue work for primitives? I thought it required a source, which for a primitive can never be null. A constantExpression may be required then.
Let's assume I'm wrong though, and defaultValue works without a source.

You would need to write @Mapping(defaultValueExpression = "java(1)"). Is this sufficient for you?

From a consumer standpoint, I would be ok with it.

In general though, I find it odd that there would be two ways to achieve the exact same mapping from an interface standpoint.

@Mapper interface MyMapper {
    @Mapping(target = "one", defaultValue = "1")
    Model fromDto(Dto dto);

    @Mapping(target = "one", defaultValueExpression = "java(1)")
    Model fromDtoButUseThisOneAsItIsMarginallyFaster(Dto dto);
}

Part of the niceness of mapstruct (and [good] generated code in general) is you don't necessarily need to really care about the code that is generated. You trust the library does what it promises (or, at least I do). I can imagine users preferring to use defaultValue = "1", it's a little clearer than the expression, especially to those newer to mapstruct -- and never looking at the code that is generated, or fiddling with defaultValueExpression to really see what the different might be. "The classes are mapped, and I didn't write it; huzzah."

So, I believe the two methods above should generate the same code. Though, I do get this is fairly low hanging fruit.

@filiphr

This comment has been minimized.

Copy link
Member

filiphr commented Mar 31, 2018

Yes defaultValue needs a source. For primitives it would work if you have a hasId for example in your object.

constant is something else. MapStruct will not use source in this case.

You are right this is fairly low hanging fruit :). I don't think that it is that difficult to add such support. We just need to allow this for java base types such as Long, Integer, Boolean etc.

In any case currently defaultValue is described as:

In case the source property is null, the provided default String value is set.

Which clearly says the default String value.

P.S. thanks for having such trust in our generated code :). We would in any case advise you from at least testing the mapping functions in your unit tests :)

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Mar 31, 2018

This is not so easy as it looks.. You'll have to figure out whether the string as constant represents a valid literal. The java spec spents several pages on what constitutes to a valid literal. It also differs per java version. _ as a grouping sign was introduced at java 7. There's no java method to validate if an input string constitutes to a valid literal (integer, long, float). So you'll have to write that yourself.

Or.. alternatively you leave it put to JDK and get a compilation failure afterwards.

@twentylemon

This comment has been minimized.

Copy link
Contributor Author

twentylemon commented Mar 31, 2018

Integer.parseInt (or equivalent for other numeric primitives) should be able to do all the heavy lifting for you. They all throw a NumberFormatException, you can catch it during the compilation and produce a meaningful error. It doesn't allow any valid literal (you can't use "1_000", that throws), but the overall behaviour would be the same as it is currently -- just with a compile time error instead of runtime.

Boolean and Character would require special handling.
Though Boolean.parseBoolean("anythingButTrue") returns false gracefully, imo it'd be better to produce an error if the string value is not "true" or "false" ignoring case.
Character, you'd just have to test the length is 1.

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Mar 31, 2018

@twentylemon

This comment has been minimized.

Copy link
Contributor Author

twentylemon commented Mar 31, 2018

Are those supported now? By mapstruct, I mean.

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Mar 31, 2018

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Apr 1, 2018

ok.. you can also specify a radix in parseXYZ

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 1, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 1, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 1, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 2, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 2, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 3, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 8, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 9, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 10, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 20, 2018

@filiphr filiphr modified the milestones: 1.3.x, 1.3.0.Beta1 Apr 22, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 23, 2018

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Apr 25, 2018

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Apr 25, 2018

Solved..

@sjaakd sjaakd closed this Apr 25, 2018

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Apr 25, 2018

@twentylemon .. fixed and merged to the master. Have a shot at it if you want..

@twentylemon

This comment has been minimized.

Copy link
Contributor Author

twentylemon commented Apr 29, 2018

@sjaakd Thanks for the update! The code is much more in depth than I was expecting -- looks good though. Radix is a neat feature to tack on.

@sjaakd

This comment has been minimized.

Copy link
Contributor

sjaakd commented Apr 30, 2018

@twentylemon Theres one more PR coming in this series on more detailed error messages: #1466 . That should make it complete. I'm not in favour of creating unexpected compilation or even runtime problems because of MapStruct generated code. Hence, I tried to follow the JDK as much as is possible (without doing the work of the compiler too much). Brought me some nice insides in the JLS though (not as consistent as you might expect 😄 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.