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.

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

Lazy initialize deprecation logger in parser #26210

Merged
merged 2 commits into from Aug 15, 2017

Conversation

@jasontedor
Copy link
Member

commented Aug 14, 2017

The deprecation logger in AbstractXContentParser is static. This is done for performance reasons, to avoid constructing a deprecation logger for every parser of which there can be many (e.g., one for every document when scripting). This is fine, but the static here is a problem because it means we touch loggers before logging is initialized (when constructing a list setting in Environment which is a precursor to initializing logging). Therefore, to maintain the previous change (not constructing a parser for every instance) but avoiding the problems with static, we have to lazy initialize here. This is not perfect, there is a volatile read behind the scenes. This could be avoided (e.g., by not using set once) but I prefer the safety that set once provides. I think this should be the approach unless it otherwise proves problematic.

Relates #25879

Lazy initialize deprecation logger in parser
The deprecation logger in AbstractXContentParser is static. This is done
for performance reasons, to avoid constructing a deprecation logger for
every parser of which there can be many (e.g., one for every document
when scripting). This is fine, but the static here is a problem because
it means we touch loggers before logging is initialized (when
constructing a list setting in Environment which is a precursor to
initializing logging). Therefore, to maintain the previous change (not
constructing a parser for every instance) but avoiding the problems with
static, we have to lazy initialize here. This is not perfect, there is a
volatile read behind the scenes. This could be avoided (e.g., by not
using set once) but I prefer the safety that set once provides. I think
this should be the approach unless it otherwise proves problematic.
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2017

Note that this is similar to the approach in Setting introduced in #25474 for exactly the same reason as here.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2017

@prog8 Would you be able to test this implementation against the case where you were previously encountering performance issues with the deprecation loggers from content parsers?

@rjernst
Copy link
Member

left a comment

LGTM

Merge branch '5.6' into static-deprecation-logger
* 5.6:
  Allow not configure logging without config
  Snapshot/Restore: Ensure that shard failure reasons are correctly stored in CS (#26127)
  Update reference from DateHistogram to Histogram (#26169)

@jasontedor jasontedor merged commit d0292f0 into elastic:5.6 Aug 15, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor deleted the jasontedor:static-deprecation-logger branch Aug 15, 2017

// do not use this field directly, use AbstractXContentParser#getDeprecationLogger
private static final SetOnce<DeprecationLogger> deprecationLogger = new SetOnce<>();

private static DeprecationLogger getDeprecationLogger() {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 15, 2017

Contributor

any reason we didn't use the holder idiom here?

ie:

private static class Holder {
  static final DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(AbstractXContentParser.class));
}

private static DeprecationLogger getDeprecationLogger() {
  return Holder.deprecationLogger;
}

This comment has been minimized.

Copy link
@jasontedor

jasontedor Aug 15, 2017

Author Member

We can do that too, it's better, I'll open a PR later.

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 15, 2017

Contributor

++ thanks

This comment has been minimized.

Copy link
@jasontedor

jasontedor Aug 15, 2017

Author Member

I opened #26218.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.