belliottsmith commented on code in PR #33: URL: https://github.com/apache/cassandra-accord/pull/33#discussion_r1103407378
########## accord-core/src/main/java/accord/local/ImmutableState.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package accord.local; + +/** + * Since metadata instances are immutable, there can be several copies floating around in memory, all but one + * of which is stale. This can lead to difficult to diagnose bugs, especially when an implementation side cache + * is involved. + * The ImmutableState base class defines the various lifecycle stage and helps restrict what the user does with it, + * with the goal or throwing an exception as soon as an illegal usage occurs, instead of after. + * + * An up to date instances can have one of 2 statuses, DORMANT or ACTIVE. An instance is only ACTIVE in the time + * between when the safe store for it's current operation is created, and closed, and is otherwise marked DORMANT. + * While DORMANT, it cannot be read from or updated. This is to help detect leaked instances being read from or + * updated when they shouldn't be. + * + * Once an update has created a superseding instance, the original instance is marked SUPERSEDED. It can then be + * marked CLEANING_UP, during which time its fields can be read to assist in determining what has been updated. + * From CLEANING_UP, an instance can be marked INVALIDATED to indicate that any access is illegal. An instance + * marked SUPERSEDED can also go directly to INVALIDATED, in the case of intermediate updates. + */ +public abstract class ImmutableState Review Comment: Awesome, I'll try to take a proper look over the weekend, or else Monday. > converted to mutable command attributes I think this is a huge improvement, though. I think we should shrink the boilerplate to {{.mutable()}}. I think we should also still consider removing the mutable step entirely - at least in the common path. This approach requires two copies of the sets we are updating, for instance, which is a cost we can avoid with a direct factory. It also probably makes the optimiser work harder, as each time we maybe make mutable it has to compile multiple branches that might involve copying a bunch of fields, making our methods larger and maybe impacting things like inlining. I think, surprisingly, there aren't actually usually many fields updated and the ones we update are quite consistent. Basically it mostly happens in `set`, so we want to update `route`, `progressKey` and then whichever fields you actually need for the new state (some of deps, txn, executeAt usually). `updateHomeKey` is basically a no-op for most paths. I think it could make sense to keep the mutable approach for the less common updates (e.g. updating home key or progress key, which usually won't be set by themselves), and have dedicated methods for the more common ones. If you take a look at my suggestions patch, `Commands.update` shows the main scope of the change. It can probably be cleaned up further based on your recent improvements. If you're amenable to it, I can try to rebase on what you've done here and clean it up. ########## accord-core/src/main/java/accord/local/ImmutableState.java: ########## @@ -0,0 +1,202 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package accord.local; + +/** + * Since metadata instances are immutable, there can be several copies floating around in memory, all but one + * of which is stale. This can lead to difficult to diagnose bugs, especially when an implementation side cache + * is involved. + * The ImmutableState base class defines the various lifecycle stage and helps restrict what the user does with it, + * with the goal or throwing an exception as soon as an illegal usage occurs, instead of after. + * + * An up to date instances can have one of 2 statuses, DORMANT or ACTIVE. An instance is only ACTIVE in the time + * between when the safe store for it's current operation is created, and closed, and is otherwise marked DORMANT. + * While DORMANT, it cannot be read from or updated. This is to help detect leaked instances being read from or + * updated when they shouldn't be. + * + * Once an update has created a superseding instance, the original instance is marked SUPERSEDED. It can then be + * marked CLEANING_UP, during which time its fields can be read to assist in determining what has been updated. + * From CLEANING_UP, an instance can be marked INVALIDATED to indicate that any access is illegal. An instance + * marked SUPERSEDED can also go directly to INVALIDATED, in the case of intermediate updates. + */ +public abstract class ImmutableState Review Comment: Awesome, I'll try to take a proper look over the weekend, or else Monday. > converted to mutable command attributes I think this is a huge improvement - though I think we should shrink the boilerplate to {{.mutable()}}. I think we should also still consider removing the mutable step entirely - at least in the common path. This approach requires two copies of the sets we are updating, for instance, which is a cost we can avoid with a direct factory. It also probably makes the optimiser work harder, as each time we maybe make mutable it has to compile multiple branches that might involve copying a bunch of fields, making our methods larger and maybe impacting things like inlining. I think, surprisingly, there aren't actually usually many fields updated and the ones we update are quite consistent. Basically it mostly happens in `set`, so we want to update `route`, `progressKey` and then whichever fields you actually need for the new state (some of deps, txn, executeAt usually). `updateHomeKey` is basically a no-op for most paths. I think it could make sense to keep the mutable approach for the less common updates (e.g. updating home key or progress key, which usually won't be set by themselves), and have dedicated methods for the more common ones. If you take a look at my suggestions patch, `Commands.update` shows the main scope of the change. It can probably be cleaned up further based on your recent improvements. If you're amenable to it, I can try to rebase on what you've done here and clean it up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
