Performance optimization for primitive Dynamic***Property classes #369
Conversation
- Added a simple benchmark to compare the implementations. - Added an attempt at DynamicPrimitiveBooleanProperty which (unlike my CachedProperties) extends the normal PropertyWrapper.
… values Changed tack to doing this to the existing classes instead of adding new implementations, so that existing users of get the benefits too. Added some tests to verify that updating of property values also updates the cached primitives.
|
archaius-pull-requests #324 FAILURE |
|
NetflixOSS » archaius » archaius-pull-requests #159 FAILURE |
|
archaius-pull-requests #325 SUCCESS |
|
NetflixOSS » archaius » archaius-pull-requests #160 FAILURE |
| * @return | ||
| */ | ||
| private boolean chooseValue() { | ||
| Boolean propValue = this.prop == null ? null : this.prop.getBoolean(); |
There was a problem hiding this comment.
The null check for the prop field is unnecessary.
There was a problem hiding this comment.
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.
Actually, all of the overhead does seem to be caused by this. I've added another benchmark comparison to the CachedPropertiesPerfTest to illustrate. eg:
Original DynamiceBooleanProperty totalled 7087 ms.
New DynamicBooleanProperty totalled 1252 ms.
New DynamicBooleanProperty with old get() totalled 7129 ms.
There was a problem hiding this comment.
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.
Ah I see what you mean now. Sorry for the confusion. I'll do that now, thanks.
- Replaced usage of the callback mechanism with overriding the propertyChanged() method. - Removed unnecessary null check. - Added a comparison with previous get() method impl in CachedPropertiesPerfTest.
|
archaius-pull-requests #326 SUCCESS |
|
NetflixOSS » archaius » archaius-pull-requests #161 FAILURE |
…have same as the previous get() method
|
archaius-pull-requests #327 SUCCESS |
|
LGTM |
|
NetflixOSS » archaius » archaius-pull-requests #162 FAILURE |
|
|
||
| @Override | ||
| protected void propertyChanged() { | ||
| super.propertyChanged(); |
There was a problem hiding this comment.
I'm a little wary of calling super here. It's a no-op there at super, so overhead is less of concern. But it maybe confusing for people down the road who might expect if they make the change in super, it would take effect in subclasses as well.
There was a problem hiding this comment.
Yeah thats true. Will change that now.
…amic*Property classes
|
archaius-pull-requests #329 SUCCESS |
|
NetflixOSS » archaius » archaius-pull-requests #164 FAILURE |
…hanging the existing Dynamic*Property classes
|
archaius-pull-requests #342 SUCCESS |
|
NetflixOSS » archaius » archaius-pull-requests #176 FAILURE |
…asses, as no longer needed
|
archaius-pull-requests #343 SUCCESS |
|
NetflixOSS » archaius » archaius-pull-requests #177 FAILURE |
| @@ -39,4 +39,4 @@ public double get() { | |||
| public Double getValue() { | |||
| return get(); | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
what're all these format changes? can we get rid of those format changes only files?
This is an optimization to reduce the cpu hotspots I was seeing under load with usage of DynamicBooleanProperty/DynamicIntProperty getValue implementation.
As we use DynamicBooleanProperty's very frequently for feature-flagging in zuul, I was seeing this quite high up when profiling zuul under high load. It seems to be caused by overhead around autoboxing of the value objects to primitives (although I haven't verified this).
I initially added a helper class in zuul for this - https://github.com/Netflix/zuul/blob/2.x/zuul-core/src/main/java/com/netflix/zuul/properties/CachedProperties.java - but would like to apply the same principle in archaius itself, and so have done this PR.
The PR basically changes the Dynamic{Primitive}Property classes to store the cached primitive value, and update it using the callback mechanism. And then change the existing get() method to return this cached value instead of boxing the property value object.
I've included a basic benchmarking class that compares performance of the old and new implementations for boolean. See CachedPropertiesPerfTest.