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

CAMEL-11420-Add contains ignore case operator to simple language #1773

Closed
wants to merge 1 commit into from
Closed

CAMEL-11420-Add contains ignore case operator to simple language #1773

wants to merge 1 commit into from

Conversation

onderson
Copy link
Contributor

No description provided.

snurmine
snurmine reviewed Jun 19, 2017
View reviewed changes
camel-core/src/main/java/org/apache/camel/util/StringHelper.java

for (int i = src.length() - length; i >= 0; i--) {
// Quick check before calling the more expensive regionMatches() method:
final char ch = src.charAt(i);
Copy link
Contributor

@snurmine snurmine Jun 19, 2017 •
edited

Choose a reason for hiding this comment

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

According to camel-jmh using four test String you provided, plain regionMatches might be the fastest; Needs to run more iterations probably to get error smaller.
Mode Cnt Score Error Units
only src.regionMatches avgt 20 4937.268 ± 106.718 us/op
empty check+src.regionMatches avgt 20 4889.541 ± 38.962 us/op
with quick check avgt 20 7250.833 ± 49.024 us/op

Probably should test with cases where there are equal number of falses and trues. Result's ofc depend on the distribution of the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with src=aaa and str is aa.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, seems to wotk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without quick check

Run complete. Total time: 00:00:28

Benchmark Mode Cnt Score Error Units
ContainsIgnoreCaseTest.benchmark thrpt 2 ≈ 10⁻⁵ ops/us
ContainsIgnoreCaseTest.benchmark avgt 2 250265,305 us/op
ContainsIgnoreCaseTest.benchmark sample 20 226072,986 ± 11368,864 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.00 sample 207618,048 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.50 sample 223608,832 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.90 sample 247699,866 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.95 sample 248499,405 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.99 sample 248512,512 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.999 sample 248512,512 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.9999 sample 248512,512 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p1.00 sample 248512,512 us/op
ContainsIgnoreCaseTest.benchmark ss 2 525478,432 us/op

With quick check

Run complete. Total time: 00:00:27

Benchmark Mode Cnt Score Error Units
ContainsIgnoreCaseTest.benchmark thrpt 2 ≈ 10⁻⁵ ops/us
ContainsIgnoreCaseTest.benchmark avgt 2 250323,754 us/op
ContainsIgnoreCaseTest.benchmark sample 20 205455,360 ± 19499,901 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.00 sample 180879,360 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.50 sample 198443,008 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.90 sample 248879,514 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.95 sample 258827,878 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.99 sample 259260,416 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.999 sample 259260,416 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.9999 sample 259260,416 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p1.00 sample 259260,416 us/op
ContainsIgnoreCaseTest.benchmark ss 2 254005,954 us/op

There is a slight difference.
I don't think jmh results are so accurate to measure such a single line. for now i'll prefer keeping it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need are more iterartions since error is so large.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With quick check
10 Warmup / 20 Measurement Iterations

Run complete. Total time: 00:02:54

Benchmark Mode Cnt Score Error Units
ContainsIgnoreCaseTest.benchmark thrpt 20 ≈ 10⁻⁵ ops/us
ContainsIgnoreCaseTest.benchmark avgt 20 256533,689 ± 31954,114 us/op
ContainsIgnoreCaseTest.benchmark sample 206 206790,895 ± 5120,475 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.00 sample 162791,424 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.50 sample 203685,888 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.90 sample 237004,390 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.95 sample 245760,000 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.99 sample 263661,814 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.999 sample 307757,056 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.9999 sample 307757,056 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p1.00 sample 307757,056 us/op
ContainsIgnoreCaseTest.benchmark ss 20 232980,729 ± 17232,308 us/op

Without quick check

Run complete. Total time: 00:02:56

Benchmark Mode Cnt Score Error Units
ContainsIgnoreCaseTest.benchmark thrpt 20 ≈ 10⁻⁵ ops/us
ContainsIgnoreCaseTest.benchmark avgt 20 256596,411 ± 17866,599 us/op
ContainsIgnoreCaseTest.benchmark sample 196 228584,218 ± 5345,551 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.00 sample 190316,544 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.50 sample 224133,120 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.90 sample 258736,128 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.95 sample 273704,550 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.99 sample 298922,803 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.999 sample 301465,600 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p0.9999 sample 301465,600 us/op
ContainsIgnoreCaseTest.benchmark:benchmark·p1.00 sample 301465,600 us/op
ContainsIgnoreCaseTest.benchmark ss 20 257154,783 ± 18134,146 us/op

Copy link
Contributor Author

Choose a reason for hiding this comment

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

still no significant change. i guess it's not easy to measure that simple check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a bug in my tests, that is why results were better. It seems that this optimization is beneficial.

davsclaus
davsclaus reviewed Jun 21, 2017
View reviewed changes
camel-core/src/main/java/org/apache/camel/builder/PredicateBuilder.java
return ObjectHelper.containsIgnoreCase(leftValue, rightValue);
}

protected String getOperationText() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be ~~ or contains ignore case etc

davsclaus
davsclaus reviewed Jun 21, 2017
View reviewed changes
tests/camel-jmh/src/test/java/org/apache/camel/itest/jmh/ContainsIgnoreCaseTest.java Outdated
import org.openjdk.jmh.runner.options.TimeValue;

/**
* Tests the {@link DefaultUuidGenerator}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is wrong ;)

@onderson
Copy link
Contributor Author

thanks for reviewing 👍

@davsclaus
Copy link
Contributor

@onders86 looks good, you also need to update the docs at
https://github.com/apache/camel/blob/master/camel-core/src/main/docs/simple-language.adoc#operator-support

To add the new operator.

After that its +1 and it can be merged. Mind that its a good idea to rebase after a pull/merge so the git tree is clean by applying the merges on top of the latest code.

git pull https://github.com/onders86/camel CAMEL-11420
git pull --rebase

@onderson
CAMEL-11420-Add contains ignore case operator to simple language
fb1a1a8 
CAMEL-11420-add jmh test

CAMEL-11420 - null check added for containsIgnoreCase in StringHelper

CAMEL-11420 - CR fixes

CAMEL-11420- update simple-language.adoc for contains ignore case
@onderson onderson closed this Jun 22, 2017
@onderson
Copy link
Contributor Author

Merged #1773

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

@davsclaus davsclaus davsclaus left review comments

@snurmine snurmine snurmine left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@onderson @davsclaus @snurmine