Skip to content

Comments

fix unnecessary logger creation#27349

Merged
jasontedor merged 1 commit intoelastic:masterfrom
tinder-xli:fix-logger
Nov 11, 2017
Merged

fix unnecessary logger creation#27349
jasontedor merged 1 commit intoelastic:masterfrom
tinder-xli:fix-logger

Conversation

@tinder-xli
Copy link
Contributor

During our perf test against elasticsearch, we noticed two synchronized block in the call stack of fetching doc values, which i think is necessary and cause a serious gc issue for us, especially when "size" param is large and fetch docvalue fields.

Logger object in DocValuesIndexFieldData.java class
Each doc value fetch will essentially create an instance of DocValuesIndexFieldData class, which will create a logger object in its constructor. However in PrefixLogger class, there is a synchronized block around marker which will block the initialization of DocValuesIndexFieldData class. We noticed a lot of thread block in PrefixLogger from JFR recording, which we believe is the root cause for super long ParNew GC:
screen shot 2017-11-10 at 4 07 06 pm

We noticed that among all the classes which extend DocValuesIndexFieldData, only SortedSetDVOrdinalsIndexFieldData class actually uses the logger, thus we are proposing moving the logger creation to this class.
We also noticed that there are quite a few other places where loggers get initialized in constructor, e.g. BulkRequestHandler.java and AbstractComponent.java. We think it could also potentially be an issue if lots of instances of such classes are created in a short period of time and suggest revisit such use cases.

gradle test all passed in my local.

For reference, below is a sample query we used for testing:

{
  "stored_fields": "_none_",
  "docvalue_fields": ["user_number"],
  "size": 33000,
  "query": {
    "bool": {
    	 "must": [
    	   {
    	   	"match_all":{}
    	   }
    	 ]
    }
  }
}

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@tinder-xli
Copy link
Contributor Author

@jasontedor

@dnhatn
Copy link
Member

dnhatn commented Nov 11, 2017

@elasticmachine please test this.

@dnhatn dnhatn requested a review from jasontedor November 11, 2017 00:59
@jasontedor
Copy link
Member

jasontedor commented Nov 11, 2017

We also noticed that there are quite a few other places where loggers get initialized in constructor, e.g. BulkRequestHandler.java and AbstractComponent.java. We think it could also potentially be an issue if lots of instances of such classes are created in a short period of time and suggest revisit such use cases.

This is not expected for either of these; it is expected that a client application using the bulk processor would not be creating many instances of bulk request handler, especially in a short period of time.

The same is true for abstract component, these are not oft-creates instances (think at the level of a system component, like an internal service).

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@jasontedor jasontedor merged commit 1e99195 into elastic:master Nov 11, 2017
jasontedor pushed a commit that referenced this pull request Nov 11, 2017
This commit removes an unnecessary logger instance creation from the
constructor for doc values field data. This construction is expensive
for this oft-created class because of a synchronized block in the
constructor for the logger.

Relates #27349
jasontedor pushed a commit that referenced this pull request Nov 11, 2017
This commit removes an unnecessary logger instance creation from the
constructor for doc values field data. This construction is expensive
for this oft-created class because of a synchronized block in the
constructor for the logger.

Relates #27349
@jasontedor jasontedor added :Search/Search Search-related issues that do not fall into other categories >enhancement backport pending v5.6.5 v6.0.1 v6.1.0 v7.0.0 labels Nov 11, 2017
jasontedor pushed a commit that referenced this pull request Nov 14, 2017
This commit removes an unnecessary logger instance creation from the
constructor for doc values field data. This construction is expensive
for this oft-created class because of a synchronized block in the
constructor for the logger.

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

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories v5.6.5 v6.0.1 v6.1.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants