-
Notifications
You must be signed in to change notification settings - Fork 480
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
Performance optimization for primitive Dynamic***Property classes #369
Changes from 4 commits
5abf02a
dad8bb0
287e179
67a75cb
d2499d2
91eef78
7e7e845
cc10674
d4a233e
2de5ae0
d3eefc7
debec15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,14 +23,41 @@ | |
* | ||
*/ | ||
public class DynamicBooleanProperty extends PropertyWrapper<Boolean> { | ||
|
||
protected volatile boolean primitiveValue; | ||
|
||
public DynamicBooleanProperty(String propName, boolean defaultValue) { | ||
super(propName, Boolean.valueOf(defaultValue)); | ||
|
||
// Set the initial value of the cached primitive value. | ||
this.primitiveValue = chooseValue(); | ||
|
||
// Add a callback to update the cached primitive value when the property is changed. | ||
this.prop.addCallback(new Runnable() { | ||
@Override | ||
public void run() { | ||
primitiveValue = chooseValue(); | ||
} | ||
}); | ||
} | ||
|
||
/** | ||
* Get the current value from the underlying DynamicProperty | ||
* | ||
* @return | ||
*/ | ||
private boolean chooseValue() { | ||
Boolean propValue = this.prop == null ? null : this.prop.getBoolean(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The null check for the prop field is unnecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method could be just the previous body of the get method: return prop.getBoolean(defaultValue).booleanValue(). The boxing and unboxing that will occur when the property value changes will be off the hot-path and a minor overhead for clearer code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, all of the overhead does seem to be caused by this. I've added another benchmark comparison to the CachedPropertiesPerfTest to illustrate. eg:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't suggesting reverting the change to the get method, which is what your new test is effectively doing, but rather to put the old code from the get method into the new chooseValue method. There it would be execute just once per property value change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see what you mean now. Sorry for the confusion. I'll do that now, thanks. |
||
return propValue == null ? defaultValue : propValue.booleanValue(); | ||
} | ||
|
||
/** | ||
* Get the current cached value. | ||
* | ||
* @return | ||
*/ | ||
public boolean get() { | ||
return prop.getBoolean(defaultValue).booleanValue(); | ||
return primitiveValue; | ||
} | ||
@Override | ||
public Boolean getValue() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
package com.netflix.config; | ||
|
||
/** | ||
* Simple benchmark impl to compare the caching of primitive boolean value in DynamicBooleanProperty with | ||
* the original implementation. | ||
* | ||
* @author Mike Smith | ||
* Date: 11/25/15 | ||
*/ | ||
public class CachedPropertiesPerfTest | ||
{ | ||
public static void main(String[] args) | ||
{ | ||
try { | ||
long loopCount = 4000000000l; | ||
|
||
// Run twice. | ||
runTest(loopCount); | ||
runTest(loopCount); | ||
} | ||
catch(Exception e) { | ||
e.printStackTrace(); | ||
} | ||
} | ||
|
||
private static void runTest(long loopCount) | ||
{ | ||
// Warmup and then run benchmark. | ||
runNewDynamicBooleanPropertyTest(10000); | ||
long durationPrimitive = runNewDynamicBooleanPropertyTest(loopCount); | ||
|
||
// Warmup and then run benchmark. | ||
runOldDynamicBooleanPropertyTest(10000); | ||
long durationOriginal = runOldDynamicBooleanPropertyTest(loopCount); | ||
|
||
System.out.println("#####################"); | ||
System.out.println("Original DynamiceBooleanProperty totalled " + durationOriginal + " ms."); | ||
System.out.println("New DynamicBooleanProperty totalled " + durationPrimitive + " ms."); | ||
} | ||
|
||
private static long runOldDynamicBooleanPropertyTest(long loopCount) | ||
{ | ||
OriginalDynamicBooleanProperty prop = new OriginalDynamicBooleanProperty("zuul.test.cachedprops.original", true); | ||
|
||
long startTime = System.currentTimeMillis(); | ||
|
||
for (long i=0; i<loopCount; i++) { | ||
prop.get(); | ||
} | ||
|
||
return System.currentTimeMillis() - startTime; | ||
} | ||
|
||
private static long runNewDynamicBooleanPropertyTest(long loopCount) | ||
{ | ||
DynamicBooleanProperty prop = | ||
new DynamicBooleanProperty("zuul.test.cachedprops.new", true); | ||
|
||
long startTime = System.currentTimeMillis(); | ||
|
||
for (long i=0; i<loopCount; i++) { | ||
prop.get(); | ||
} | ||
|
||
return System.currentTimeMillis() - startTime; | ||
} | ||
|
||
/** | ||
* This is a copy of the DynamicBooleanProperty class from before I made the performance optimization to it. | ||
* It's here so I can still do a back-to-back comparison of performance. | ||
*/ | ||
static class OriginalDynamicBooleanProperty extends PropertyWrapper<Boolean> { | ||
|
||
public OriginalDynamicBooleanProperty(String propName, boolean defaultValue) { | ||
super(propName, Boolean.valueOf(defaultValue)); | ||
} | ||
|
||
public boolean get() { | ||
return prop.getBoolean(defaultValue).booleanValue(); | ||
} | ||
|
||
@Override | ||
public Boolean getValue() { | ||
return get(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding this callback here, you could just override either of the propertyChanged methods and then update the PropertyWrapper class to not add this class to the SUBCLASSES_WITH_NO_CALLBACK map. This would be cleaner and probably result in less overall code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is a much better way of doing it.
@howardyuan - i've made this change to the PR. Does this satisfy your concerns around my previous usage of the callbacks?