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

Hot methods redux #19016

Merged
merged 7 commits into from
Jun 22, 2016
Merged

Hot methods redux #19016

merged 7 commits into from
Jun 22, 2016

Conversation

jasontedor
Copy link
Member

This pull request addresses some additional hot methods that could not
be inlined due to exceeding MaxFreqInlineSize. In particular, this
pull request addresses all hot methods in the indexing path of the Rally
default benchmarking run:

org.elasticsearch.action.search.AbstractSearchAsyncAction::<init> (355 bytes)   hot method too big
org.elasticsearch.action.search.TransportSearchAction::doExecute (380 bytes)   hot method too big
org.elasticsearch.action.support.replication.ReplicationOperation::execute (353 bytes)   hot method too big
org.elasticsearch.common.io.stream.StreamInput::readGenericValue (497 bytes)   hot method too big
org.elasticsearch.common.io.stream.StreamOutput::writeGenericValue (799 bytes)   hot method too big
org.elasticsearch.index.engine.InternalEngine::innerIndex (465 bytes)   hot method too big
org.elasticsearch.tasks.TaskManager::register (336 bytes)   hot method too big

Relates #16725

@jasontedor
Refactor InternalEngine inner methods
4f49a26 
This commit refactors InternalEngine#innerIndex and
InternalEngine#innerDelete to collapse some common logic into a single
method. This has the advantage that it shrinks the bytecode size of
InternalEngine#innerIndex so that it can be inlined.
jpountz
jpountz reviewed Jun 22, 2016
View reviewed changes
core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java
byte[] value = new byte[bytesSize];
readBytes(value, 0, bytesSize);
return value;
return fastReadByteArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

why "fast"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jpountz Because there is already a method named readByteArray and that method reads a single byte at a time but the method here uses readBytes which can be optimized to read a chunk of bytes. I think that these methods can be combined but will address that in a follow-up if so to keep this PR purely about inlining.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was addressed #19023.

@jpountz
Copy link
Contributor

jpountz commented Jun 22, 2016

Looks fine to me, I think it even makes things slightly more readable.

@jasontedor
Copy link
Member Author

I think it even makes things slightly more readable

Agree! Thanks for reviewing!

@jasontedor jasontedor merged commit 9d6d815 into elastic:master Jun 22, 2016
@jasontedor jasontedor deleted the hot-methods-redux branch June 22, 2016 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>enhancement v5.0.0-alpha4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasontedor @jpountz