You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by njhill <gi...@git.apache.org> on 2018/01/02 20:36:48 UTC
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
GitHub user njhill opened a pull request:
https://github.com/apache/curator/pull/250
[CURATOR-447] TreeCache: Improve memory usage and thread safety
Jira https://issues.apache.org/jira/browse/CURATOR-374 reduced per-node memory usage in `TreeCache`. It can be improved further via removal of the `nodeState` field - its `LIVE` state corresponds exactly to the adjacent `childData` field being non-null, and a sentinel `ChildData` value can be used for the `DEAD` state. This simplification also reduces the room for bugs and state inconsistencies.
Other improvements included:
* A further simplification to have `TreeNode` extend `AtomicReference`, which obviates the need for an explicit `childData` field
* More robust cache update logic (in get-children and get-data event callbacks)
* Avoid overhead of incrementing/decrementing the `outstandingOps` atomic integer post-initialization
See corresponding JIRA: [CURATOR-447](https://issues.apache.org/jira/browse/CURATOR-447)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/njhill/curator CURATOR-447
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/250.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #250
----
commit 1c7be856fb514015286855f1ad9fd89069c32906
Author: nickhill <ni...@...>
Date: 2017-03-26T20:19:40Z
remove nodeState field, improve thread safety of cache updates
also simplify by having TreeNode extend AtomicReference<ChildData>
commit 67210556919d7f822c431597245c4fa7246901dd
Author: nickhill <ni...@...>
Date: 2017-03-26T20:20:32Z
avoid incrementing/decrementing outstandingOps post-initialization
no need to incur the synchronization overhead
commit a5a680d50e50a0a804caa875a53e2ee8d9a268f9
Author: nickhill <ni...@...>
Date: 2018-01-02T16:36:20Z
remove superfluous braces
----
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/250
@njhill this looks good to me with a few changes. I pushed one extra commit onto https://github.com/apache/curator/tree/CURATOR-447 for you to look at. If you agree with my changes (some of them are to conform to the project style) please sync your branch to mine and I'll merge this.
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167677124
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -436,41 +432,28 @@ else if ( event.getResultCode() == KeeperException.Code.NONODE.intValue() )
case GET_DATA:
if ( event.getResultCode() == KeeperException.Code.OK.intValue() )
{
- ChildData toPublish = new ChildData(event.getPath(), newStat, event.getData());
- ChildData oldChildData;
- if ( cacheData )
- {
- oldChildData = childDataUpdater.getAndSet(this, toPublish);
- }
- else
- {
- oldChildData = childDataUpdater.getAndSet(this, new ChildData(event.getPath(), newStat, null));
- }
-
- boolean added;
- if (parent == null) {
- // We're the singleton root.
- added = nodeStateUpdater.getAndSet(this, NodeState.LIVE) != NodeState.LIVE;
- } else {
- added = nodeStateUpdater.compareAndSet(this, NodeState.PENDING, NodeState.LIVE);
- if (!added) {
- // Ordinary nodes are not allowed to transition from dead -> live;
- // make sure this isn't a delayed response that came in after death.
- if (nodeState != NodeState.LIVE) {
- return;
- }
+ String eventPath = event.getPath();
+ ChildData toPublish = new ChildData(eventPath, newStat, event.getData());
+ ChildData toUpdate = cacheData ? toPublish : new ChildData(eventPath, newStat, null);
+ while (true) {
+ final ChildData oldChildData = childData;
+ if ( (isLive(oldChildData) && newStat.getMzxid() <= oldChildData.getStat().getMzxid())
+ // Ordinary nodes are not allowed to transition from dead -> live;
+ // make sure this isn't a delayed response that came in after death.
+ || (parent != null && oldChildData == DEAD) ) {
+ break;
}
- }
-
- if ( added )
- {
- publishEvent(TreeCacheEvent.Type.NODE_ADDED, toPublish);
- }
- else
- {
- if ( oldChildData == null || oldChildData.getStat().getMzxid() != newStat.getMzxid() )
+ if ( childDataUpdater.compareAndSet(this, oldChildData, toUpdate) )
{
- publishEvent(TreeCacheEvent.Type.NODE_UPDATED, toPublish);
+ if ( !isLive(oldChildData) )
+ {
+ publishEvent(TreeCacheEvent.Type.NODE_ADDED, toPublish);
+ }
+ else if ( oldChildData.getStat().getMzxid() != newStat.getMzxid() )
+ {
+ publishEvent(TreeCacheEvent.Type.NODE_UPDATED, toPublish);
+ }
+ break;
--- End diff --
I think the second check is now superfluous since we checked above.
```
if ( childDataUpdater.compareAndSet(this, oldChildData, toUpdate) )
{
publishEvent(isLive(oldChildData) ? TreeCacheEvent.Type.NODE_UPDATED : TreeCacheEvent.Type.NODE_ADDED, toPublish);
break;
}
```
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167715541
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -704,8 +687,7 @@ private TreeNode find(String findPath)
{
TreeNode childNode = entry.getValue();
ChildData childData = childNode.childData;
- // Double-check liveness after retreiving data.
--- End diff --
The reason I removed it is that unlike before it's not really "double" checking anymore against the separate `nodeState` field, there's only a single check for liveness which is now implicit in the value of `childData`. But I could well have misinterpreted the reason for comment, no problem with leaving it in!
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on the issue:
https://github.com/apache/curator/pull/250
Thanks @dragonsinth, there's of course no rush at all!
I agree that `TreeNode extends AtomicReference` isn't a "win" as such, it just looked cleaner to me than using another `AtomicReferenceFieldUpdater`. But it's likely subjective and certainly a minor thing.
The `outstandingOps` change seems clearer to me - the field has no use for most of the life of the cache and yet is incremented/decremented on every single change. It's not the CPU cycle overhead but more the (CPU) cache coherency overhead since it's an `AtomicLong`. I didn't think the change introduces any raciness - the only times when `outstandingOps` needs to be modified (during initialization or re-initialization) it will be guaranteed to be non-null.
> I would expect the memory footprint of `ConcurrentMap<String, TreeNode>` children to mostly dominate the memory overhead of our own classes
Good point. The description of [CURATOR-374](https://issues.apache.org/jira/browse/CURATOR-374) implied this might have _some_ mem overhead benefit for very large caches, but removal of the `nodeState` field was motivated more by state simplification and consolidation of atomic updates. I'll do some quick measurements but I acknowledge percentage-wise the saving might not be significant.
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167673103
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -704,8 +687,7 @@ private TreeNode find(String findPath)
{
TreeNode childNode = entry.getValue();
ChildData childData = childNode.childData;
- // Double-check liveness after retreiving data.
--- End diff --
I kind of liked this comment, although I misspelled "retrieving" here and below
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on the issue:
https://github.com/apache/curator/pull/250
@dragonsinth I've removed the `outstandingOps` change commit from the PR, and added a new commit which reverts the `TreeNode extends AtomicReference<ChildData>` change. Hopefully that helps to make this more palatable!
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/250
I'll defer to @dragonsinth
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/250
Sorry, been busy then out of town. I'll look tomorrow.
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on the issue:
https://github.com/apache/curator/pull/250
@dragonsinth any interest in these changes? Maybe I should rename the PR since as you pointed out the memory saving aspect isn't very significant (I measured about 2% on a cache of 50k empty nodes), but hopefully the state and update logic simplification is beneficial?
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/250
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167668989
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -377,15 +375,16 @@ public void processResult(CuratorFramework client, CuratorEvent event) throws Ex
Preconditions.checkState(parent == null, "unexpected EXISTS on non-root node");
if ( event.getResultCode() == KeeperException.Code.OK.intValue() )
{
- nodeStateUpdater.compareAndSet(this, NodeState.DEAD, NodeState.PENDING);
+ childDataUpdater.compareAndSet(this, DEAD, null);
wasCreated();
}
break;
case CHILDREN:
if ( event.getResultCode() == KeeperException.Code.OK.intValue() )
{
ChildData oldChildData = childData;
- if ( oldChildData != null && oldChildData.getStat().getMzxid() == newStat.getMzxid() )
+ //TODO consider doing update of cversion, pzxid, numChildren only
--- End diff --
seems reasonable to me
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167675994
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -436,41 +432,28 @@ else if ( event.getResultCode() == KeeperException.Code.NONODE.intValue() )
case GET_DATA:
if ( event.getResultCode() == KeeperException.Code.OK.intValue() )
{
- ChildData toPublish = new ChildData(event.getPath(), newStat, event.getData());
- ChildData oldChildData;
- if ( cacheData )
- {
- oldChildData = childDataUpdater.getAndSet(this, toPublish);
- }
- else
- {
- oldChildData = childDataUpdater.getAndSet(this, new ChildData(event.getPath(), newStat, null));
- }
-
- boolean added;
- if (parent == null) {
- // We're the singleton root.
- added = nodeStateUpdater.getAndSet(this, NodeState.LIVE) != NodeState.LIVE;
- } else {
- added = nodeStateUpdater.compareAndSet(this, NodeState.PENDING, NodeState.LIVE);
- if (!added) {
- // Ordinary nodes are not allowed to transition from dead -> live;
- // make sure this isn't a delayed response that came in after death.
- if (nodeState != NodeState.LIVE) {
- return;
- }
+ String eventPath = event.getPath();
+ ChildData toPublish = new ChildData(eventPath, newStat, event.getData());
+ ChildData toUpdate = cacheData ? toPublish : new ChildData(eventPath, newStat, null);
+ while (true) {
+ final ChildData oldChildData = childData;
+ if ( (isLive(oldChildData) && newStat.getMzxid() <= oldChildData.getStat().getMzxid())
+ // Ordinary nodes are not allowed to transition from dead -> live;
+ // make sure this isn't a delayed response that came in after death.
+ || (parent != null && oldChildData == DEAD) ) {
+ break;
--- End diff --
Can we break up these cases for clarity?
```
final ChildData oldChildData = childData;
// Ignore this event if we've already processed a newer update for this node.
if ( (isLive(oldChildData) && newStat.getMzxid() <= oldChildData.getStat().getMzxid()) )
{
break;
}
// Non-root nodes are not allowed to transition from dead -> live;
// make sure this isn't a delayed response that came in after death.
if ( parent != null && oldChildData == DEAD )
{
break;
}
```
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on the issue:
https://github.com/apache/curator/pull/250
@dragonsinth @Randgalt any interest in these updates? FWIW I've had them running in production for some time.
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/250
I'm sure this is probably right, but the old code has a _lot_ of miles on it. I have to read through super carefully and convince myself that there are no behavioral changes.
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167672355
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -397,14 +396,11 @@ public void processResult(CuratorFramework client, CuratorEvent event) throws Ex
break;
}
- ConcurrentMap<String, TreeNode> childMap = children;
- if ( childMap == null )
+ ConcurrentMap<String, TreeNode> childMap;
+ while ( (childMap = children) == null )
{
childMap = Maps.newConcurrentMap();
- if ( !childrenUpdater.compareAndSet(this, null, childMap) )
- {
- childMap = children;
- }
+ if ( childrenUpdater.compareAndSet(this, null, childMap) ) break;
--- End diff --
I see why you turned this into a loop (nice catch), but I think it reads a little clearly and more deliberateif you revert back to the original code and just change the `if` -> `while`. Ideally if Curator moves wholesale to Java 1.8, funcs, like:
```
ConcurrentMap<String, TreeNode> childMap = childrenUpdater.updateAndGet(this, (oldChildren) -> {
return oldChildren != null ? oldChildren : Maps.newConcurrentMap();
});
```
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167715799
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -728,13 +710,12 @@ private TreeNode find(String findPath)
public ChildData getCurrentData(String fullPath)
{
TreeNode node = find(fullPath);
- if ( node == null || node.nodeState != NodeState.LIVE )
+ if ( node == null )
{
return null;
}
ChildData result = node.childData;
- // Double-check liveness after retreiving data.
- return node.nodeState == NodeState.LIVE ? result : null;
+ return result != DEAD ? result : null;
--- End diff --
Reason was that `isLive` also checks for `null` which isn't necessary here (since we're returning `null` anyhow). But no harm in making it consistent.
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by njhill <gi...@git.apache.org>.
Github user njhill commented on the issue:
https://github.com/apache/curator/pull/250
Thanks alot @dragonsinth for going through this so carefully. I like your additional simplifications, and sorry for the project style inconsistencies that I missed!
I've pulled in your commit and added one more just to remove some parentheses no longer needed after splitting those conditions into separate `if`s.
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167733216
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -702,19 +687,18 @@ private TreeNode find(String findPath)
ImmutableMap.Builder<String, ChildData> builder = ImmutableMap.builder();
for ( Map.Entry<String, TreeNode> entry : map.entrySet() )
{
- TreeNode childNode = entry.getValue();
- ChildData childData = childNode.childData;
- // Double-check liveness after retreiving data.
- if ( childData != null && childNode.nodeState == NodeState.LIVE )
+ ChildData childData = entry.getValue().childData;
+ // Double-check liveness after retrieving data.
+ if ( isLive(childData) )
--- End diff --
It's subtle, but the point I'm trying to make is that you have to read `.childData` exactly once, then check the liveness, then use only that value going forward. It would be an error to remove the local variable here and reference `entry.getValue().childData` twice.
It's a "double" check because _generally_ there shouldn't be any non-LIVE children in the parent `children` map that we're iterating. But due to volatility we have to "double check" that child nodes read from the map are still alive after pulling `.childData`
---
[GitHub] curator issue #250: [CURATOR-447] TreeCache: Improve memory usage and concur...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the issue:
https://github.com/apache/curator/pull/250
Hi @njhill just got back to work from vacation, and this is a little more than I can just eyeball, I gotta think through it a bit when I have more time. I think I would like to see the removal of `nodeState` in isolation, perhaps with an explicit `PENDING` sentinel rather than just `null`.
It's not clear to me that `TreeNode extends AtomicReference` and `@Nullable outstandingOps` are wins. The first one doesn't really save any memory, and for the second one, the ongoing CPU cost to update `outstandingOps` is basically nothing, it's almost certainly immeasurably tiny, and you have to introduce racy field reads to accomplish it.
Have you done memory use measurements? I would expect the memory footprint of `ConcurrentMap<String, TreeNode> children` to mostly dominate the memory overhead of our own classes, not to mention all the Watchers.
---
[GitHub] curator pull request #250: [CURATOR-447] TreeCache: Improve memory usage and...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/250#discussion_r167673343
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -728,13 +710,12 @@ private TreeNode find(String findPath)
public ChildData getCurrentData(String fullPath)
{
TreeNode node = find(fullPath);
- if ( node == null || node.nodeState != NodeState.LIVE )
+ if ( node == null )
{
return null;
}
ChildData result = node.childData;
- // Double-check liveness after retreiving data.
- return node.nodeState == NodeState.LIVE ? result : null;
+ return result != DEAD ? result : null;
--- End diff --
shouldn't this be `isLive` for consistency?
---