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

Consider avoiding string copy on substring for ~20% url matching performance improvement #571

Closed
ahunt opened this Issue May 8, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@ahunt
Contributor

ahunt commented May 8, 2017

It turns out that Android Java does create a new String copy for String.substring():
http://androidxref.com/6.0.1_r10/xref/art/runtime/mirror/string-inl.h#188

I'd naively assumed that wasn't the case, and our UrlMatcher calls substring() a lot. We already wrap Java's String in FocusString to avoid copies when reversing Strings - we could consider avoiding String.substring() by storing our own String offsets in FocusString. (Note: Java 6 used offsets, Java 7 switched during a point release, based on reading https://dzone.com/articles/changes-stringsubstring-java-7 ).

Doing that leads to a ~20% performance improvement in UrlMatcher.matches(), based on my testing on an N6P with Android 7.1.2 (default blocklist settings, ie. ad/analytic/social trackers blocked).

Average time for matcher.matches:
before (copy on substring) = 1.19ms
after (offsets in FocusString) = 0.96ms

Note: I already have a local patch doing this, I just need to write some more tests to make sure FocusString really is bulletproof.

I don't think this is super important (we already are quite a bit faster than focus-iOS's matching algorithm), but the changes are fairly simple.

@ahunt ahunt added the code label May 8, 2017

@ahunt ahunt self-assigned this May 8, 2017

@ahunt

This comment has been minimized.

Show comment
Hide comment
@ahunt

ahunt May 8, 2017

Contributor

I've pushed a copy of my branch which contains a (non-landable) measurement patch to ahunt/matcherperf-measurements.

Contributor

ahunt commented May 8, 2017

I've pushed a copy of my branch which contains a (non-landable) measurement patch to ahunt/matcherperf-measurements.

@ahunt

This comment has been minimized.

Show comment
Hide comment
@ahunt

ahunt May 9, 2017

Contributor

I noticed that the performance numbers were a lot worse than 1 month ago: it turns out we now use testCoverageEnabled true for our debug builds. If I remove that and enable proguard, the numbers are:
before (copy on substring) = 0.42ms
after (offsets in FocusString) = 0.26ms

Contributor

ahunt commented May 9, 2017

I noticed that the performance numbers were a lot worse than 1 month ago: it turns out we now use testCoverageEnabled true for our debug builds. If I remove that and enable proguard, the numbers are:
before (copy on substring) = 0.42ms
after (offsets in FocusString) = 0.26ms

@pocmo

This comment has been minimized.

Show comment
Hide comment
@pocmo

pocmo May 9, 2017

Contributor

It's a bit strange that testCoverageEnabled does anything when we are not running tests but the app itself. I wonder if it's worth to create a separate flavor for this - but this is probably more annoying.

Contributor

pocmo commented May 9, 2017

It's a bit strange that testCoverageEnabled does anything when we are not running tests but the app itself. I wonder if it's worth to create a separate flavor for this - but this is probably more annoying.

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