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

Hot inlined methods in your area #16725

Closed
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jasontedor
Member

jasontedor commented Feb 18, 2016

An important optimization that a compiler can make is to inline
methods. The JVM JIT compiler makes decisions about inlining methods
based on method size and how hot the method is. For example, small
methods (less than 35 bytes by default, but tunable via
-XX:MaxInlineSize=N ) are automatically inlined. And hot methods are
inlined only if they are not large (less than 325 bytes by default, but
tunable via -XX:MaxFreqInlineSize=N). Hot methods that the JVM could
not inline in Elasticsearch can be found by enabling the JVM to trace
method inlining via -XX:+UnlockDiagnoticVMOptions -XX:+PrintInlining
and then looking for "hot method too big" for any methods in a
sub-package of org.elasticsearch. It is generally recommend to not
tune the inlining flags. Instead, we can enable the JVM to inline these
hot methods by breaking these methods down into smaller methods.

The methods that are hot but could not be inlined while benchmarking
Elasticsearch are:

org.elasticsearch.action.bulk.TransportShardBulkAction::shardOperationOnPrimary (1918 bytes)   hot method too big
org.elasticsearch.action.support.replication.TransportReplicationAction$PrimaryPhase::doRun (344 bytes)   hot method too big
org.elasticsearch.action.support.replication.TransportReplicationAction$ReplicationPhase::<init> (440 bytes)   hot method too big
org.elasticsearch.action.support.replication.TransportReplicationAction$ReroutePhase::doRun (739 bytes)   hot method too big
org.elasticsearch.cluster.metadata.IndexNameExpressionResolver$DateMathExpressionResolver::resolveExpression (740 bytes)   hot method too big
org.elasticsearch.cluster.metadata.IndexNameExpressionResolver$WildcardExpressionResolver::resolve (1110 bytes)   hot method too big
org.elasticsearch.cluster.metadata.MetaData::resolveIndexRouting (351 bytes)   hot method too big
org.elasticsearch.common.Base64::decode (389 bytes)   hot method too big
org.elasticsearch.common.Base64::decode4to3 (350 bytes)   hot method too big
org.elasticsearch.common.Base64::encodeBytesToBytes (456 bytes)   hot method too big
org.elasticsearch.common.breaker.ChildMemoryCircuitBreaker::addEstimateBytesAndMaybeBreak (372 bytes)   hot method too big
org.elasticsearch.common.xcontent.XContentBuilder::writeValue (1203 bytes)   hot method too big
org.elasticsearch.http.netty.NettyHttpChannel::getStatus (388 bytes)   hot method too big
org.elasticsearch.http.netty.NettyHttpChannel::sendResponse (562 bytes)   hot method too big
org.elasticsearch.index.fielddata.ordinals.OrdinalsBuilder$OrdinalsStore::addOrdinal (348 bytes)   hot method too big
org.elasticsearch.index.mapper.DocumentParser::parseObject (687 bytes)   hot method too big
org.elasticsearch.rest.support.RestUtils::decodeComponent (387 bytes)   hot method too big
org.elasticsearch.search.aggregations.AggregationPhase::execute (558 bytes)   hot method too big
org.elasticsearch.search.aggregations.bucket.terms.GlobalOrdinalsStringTermsAggregator::buildAggregation (540 bytes)   hot method too big
org.elasticsearch.search.aggregations.bucket.terms.InternalTerms::doReduce (687 bytes)   hot method too big
org.elasticsearch.search.controller.SearchPhaseController::sortDocs (673 bytes)   hot method too big
org.elasticsearch.search.fetch.FetchPhase::execute (645 bytes)   hot method too big
org.elasticsearch.search.internal.InternalSearchHit::toXContent (890 bytes)   hot method too big
org.elasticsearch.search.query.QueryPhase::execute (1174 bytes)   hot method too big

This commit refactors most of the methods that are hot but the JVM could
not inline because they are too large so that they JVM can now inline
them. Simple micro-benchmarking on a few (but not all) of these methods
showed modest performance gains on the order of 5%. This refactoring has
the additional advantage that by breaking these large methods into
smaller methods, they are simpler to understand and maintain.

The methods not addressed by this pull request are:

org.elasticsearch.cluster.metadata.IndexNameExpressionResolver$DateMathExpressionResolver::resolveExpression (740 bytes)   hot method too big
org.elasticsearch.search.aggregations.AggregationPhase::execute (558 bytes)   hot method too big
org.elasticsearch.search.aggregations.bucket.terms.GlobalOrdinalsStringTermsAggregator::buildAggregation (540 bytes)   hot method too big
org.elasticsearch.search.aggregations.bucket.terms.InternalTerms::doReduce (687 bytes)   hot method too big
org.elasticsearch.search.controller.SearchPhaseController::sortDocs (673 bytes)   hot method too big
org.elasticsearch.search.fetch.FetchPhase::execute (645 bytes)   hot method too big
org.elasticsearch.search.internal.InternalSearchHit::toXContent (890 bytes)   hot method too big
org.elasticsearch.search.query.QueryPhase::execute (1174 bytes)   hot method too big

All other methods are addressed.

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Feb 19, 2016

Member

@jasontedor out of curiosity - do you have any numbers about the performance delta between current code and your PR?

Member

bleskes commented Feb 19, 2016

@jasontedor out of curiosity - do you have any numbers about the performance delta between current code and your PR?

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Feb 19, 2016

Member

do you have any numbers about the performance delta between current code and your PR?

@bleskes I do, and there are issues which I've discussed with @danielmitterdorfer. The issue is that there is variance in the benchmark results which I found to be due to a large variance in the number of segments; this hides the impact of this change (which is expected to be small but positive).

Member

jasontedor commented Feb 19, 2016

do you have any numbers about the performance delta between current code and your PR?

@bleskes I do, and there are issues which I've discussed with @danielmitterdorfer. The issue is that there is variance in the benchmark results which I found to be due to a large variance in the number of segments; this hides the impact of this change (which is expected to be small but positive).

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Feb 23, 2016

Member

@danielmitterdorfer Is there any chance of getting this reviewed before the code base starts moving rapidly again?

Member

jasontedor commented Feb 23, 2016

@danielmitterdorfer Is there any chance of getting this reviewed before the code base starts moving rapidly again?

@danielmitterdorfer

This comment has been minimized.

Show comment
Hide comment
@danielmitterdorfer

danielmitterdorfer Feb 23, 2016

Member

I left a few minor comments here and there. I also ran the benchmarks multiple times but the changes hide in the run-to-run variance. Nevertheless, I think it is great to have shorter methods and it improves maintainability. Btw, I've also liked very much how you've split your commits for this PR.

Apart from the minor comments, LGTM.

Member

danielmitterdorfer commented Feb 23, 2016

I left a few minor comments here and there. I also ran the benchmarks multiple times but the changes hide in the run-to-run variance. Nevertheless, I think it is great to have shorter methods and it improves maintainability. Btw, I've also liked very much how you've split your commits for this PR.

Apart from the minor comments, LGTM.

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

jasontedor added a commit that referenced this pull request Feb 24, 2016

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Feb 24, 2016

Member

Thanks for the review @danielmitterdorfer; I integrated your feedback into the version that was pushed to master.

Member

jasontedor commented Feb 24, 2016

Thanks for the review @danielmitterdorfer; I integrated your feedback into the version that was pushed to master.

@jasontedor jasontedor closed this Feb 24, 2016

@jasontedor jasontedor deleted the jasontedor:hot-inlined-methods-in-your-area branch Feb 24, 2016

@clintongormley clintongormley added :Internal and removed :Core/Core labels Feb 28, 2016

@jasontedor jasontedor referenced this pull request Jun 22, 2016

Merged

Hot methods redux #19016

@danielmitterdorfer danielmitterdorfer removed their assignment Jan 20, 2017

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