Cache generic param names

mybatis / mybatis-3

MyBatis SQL mapper framework for Java

http://mybatis.github.io/mybatis-3/

Apache License 2.0

19.55k stars 12.74k forks source link

Cache generic param names #3176

Closed kang-hl closed 2 weeks ago

coveralls commented 3 weeks ago

coverage: 87.175% (+0.005%) from 87.17% when pulling a854fa8eddb45da756cc5568c76f99657bd12e41 on kang-hl:add-cache into e836410f8c4eb379549297f6f0a6ccd8ac73adcf on mybatis:master.

hazendaz commented 3 weeks ago

@harawata Any concerns here? I don't see any real difference on processing of a build along with tests from any outstanding builds. This is probably very micro here at caching so not fully seeing the value. Plus seems odd that we prep for full 255 when that number is probably more often than not too high.

harawata commented 3 weeks ago

Yeah. It is rare to see a statement with 5+ parameters.

It might be possible to cache only the first five or ten names and create the others on the fly, but it requires an extra check, so ... I'm not sure.

genericParamName = i < 10 ? CACHED[i] : PREFIX + (i + 1);

kang-hl commented 3 weeks ago

Yeah. It is rare to see a statement with 5+ parameters.

It might be possible to cache only the first five or ten names and create the others on the fly, but it requires an extra check, so ... I'm not sure.
genericParamName = i < 10 ? CACHED[i] : PREFIX + (i + 1);

Adding an environment variable to change this value may be better.

kang-hl commented 3 weeks ago

Yeah. It is rare to see a statement with 5+ parameters.

It might be possible to cache only the first five or ten names and create the others on the fly, but it requires an extra check, so ... I'm not sure.
genericParamName = i < 10 ? CACHED[i] : PREFIX + (i + 1);

Adding an environment variable to change this value may be better.

harawata commented 3 weeks ago

@kang-hl ,

I prefer not to add a new config option for this. How many parameters does your statement actually have?

kang-hl commented 3 weeks ago

@kang-hl ,

I prefer not to add a new config option for this. How many parameters does your statement actually have?

Only a few at most, but not everyone is like this. Can only add one environment variable or an extra check or keep it as is, which one do you prefer?

harawata commented 3 weeks ago

Let's just cache the first ten names. It covers more than 95%, I believe.

And we can change it anytime if we need to.

kang-hl commented 3 weeks ago

Let's just cache the first ten names. It covers more than 95%, I believe.

And we can change it anytime if we need to.

Modified.

coveralls commented 3 weeks ago

coverage: 87.175% (+0.005%) from 87.17% when pulling 859a2546696c302f4189a50799c80996acb0f079 on kang-hl:add-cache into e836410f8c4eb379549297f6f0a6ccd8ac73adcf on mybatis:master.

harawata commented 2 weeks ago

Thank you for the PR, @kang-hl ! And thank you for the review, @epochcoder !