Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Implemented new flag storage on IChemObject implementations. Flags ar…

…e now stored on a single numeric value (currently a short) and flags are accessed/mutated via bit shifting of this value. This implementation provides space saving over using a boolean array however getFlags() and setFlags() now have a overhead due to conversion from the array to the numeric value. Usage however indicates the singular setFlag/getFlag is used >1000 times where as the setFlags/getFlags is used ~50.

Also removing debug statement from AbstractChemObjectTest

Signed-off-by: Egon Willighagen <egonw@users.sourceforge.net>
  • Loading branch information...
commit 1a1b03d39e13402a222b5d1521a749fae2d98bae 1 parent 0352d30
@johnmay johnmay authored egonw committed
View
2  src/main/org/openscience/cdk/CDKConstants.java
@@ -172,7 +172,7 @@
/**
* Maximum flags array index. Please update this if the value exceeds 16 -
* the flags are currently stored as a single short value (16-bit) in the
- * ChemObject implementations.
+ * ChemObject implementations (default, silent and query).
*/
public final static int MAX_FLAG_INDEX = 12;
View
63 src/main/org/openscience/cdk/ChemObject.java
@@ -77,7 +77,7 @@
* flag array with self-defined constants (flags[VISITED] = true). 100 flags
* per object should be more than enough.
*/
- private boolean[] flags;
+ private short flags; // flags are currently stored as a single short value MAX_FLAG_INDEX < 16
/**
* The ID is null by default.
@@ -90,7 +90,6 @@
*/
public ChemObject()
{
- flags = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
chemObjectListeners = null;
properties = null;
identifier = null;
@@ -105,8 +104,7 @@ public ChemObject()
public ChemObject(IChemObject chemObject) {
// copy the flags
boolean[] oldflags = chemObject.getFlags();
- flags = new boolean[oldflags.length];
- System.arraycopy(oldflags, 0, flags, 0, flags.length);
+ flags = chemObject.getFlagValue().shortValue();
// copy the identifier
identifier = chemObject.getID();
}
@@ -301,8 +299,7 @@ public Object clone() throws CloneNotSupportedException
{
ChemObject clone = (ChemObject)super.clone();
// clone the flags
- clone.flags = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
- System.arraycopy(flags, 0, clone.flags, 0, flags.length);
+ clone.flags = getFlagValue().shortValue();
// clone the properties
if (properties != null) {
Map<Object, Object> clonedHashtable = new HashMap<Object, Object>();
@@ -363,33 +360,40 @@ public void setID(String identifier)
/**
- * Sets the value of some flag.
- *
- *@param flag_type Flag to set
- *@param flag_value Value to assign to flag
- *@see #getFlag
+ * @inheritDoc
*/
- public void setFlag(int flag_type, boolean flag_value)
+ @Override
+ public void setFlag(int mask, boolean value)
{
- flags[flag_type] = flag_value;
- notifyChanged();
+ // set/unset a bit in the flags value
+ if (value)
+ flags |= mask;
+ else
+ flags &= ~(mask);
+ notifyChanged();
}
- /**
- * Returns the value of some flag.
- *
- *@param flag_type Flag to retrieve the value of
- *@return true if the flag <code>flag_type</code> is set
- *@see #setFlag
- */
- public boolean getFlag(int flag_type)
+ /**
+ * @inheritDoc
+ */
+ @Override
+ public boolean getFlag(int mask)
{
- return flags[flag_type];
+ return (flags & mask) != 0;
}
- /**
+ /**
+ * @inheritDoc
+ */
+ @Override
+ public Short getFlagValue() {
+ return flags;
+ }
+
+
+ /**
* Sets the properties of this object.
*
*@param properties a Hashtable specifying the property values
@@ -414,7 +418,8 @@ public void setProperties(Map<Object,Object> properties)
* @see #getFlags
*/
public void setFlags(boolean[] flagsNew){
- flags=flagsNew;
+ for(int i = 0; i < flagsNew.length ; i++)
+ setFlag(CDKConstants.FLAG_MASKS[i], flagsNew[i]);
notifyChanged();
}
@@ -425,7 +430,13 @@ public void setFlags(boolean[] flagsNew){
*@see #setFlags
*/
public boolean[] getFlags(){
- return(flags);
+ // could use a list a invoke .toArray() on the return
+ boolean[] flagArray = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
+ for(int i = 0 ; i < CDKConstants.FLAG_MASKS.length; i++){
+ int mask = CDKConstants.FLAG_MASKS[i];
+ flagArray[i] = getFlag(mask);
+ }
+ return flagArray;
}
/**
View
1  src/main/org/openscience/cdk/config/NaturalElement.java
@@ -60,6 +60,7 @@ protected NaturalElement(String element, Integer atomicNumber) {
// unsupported methods
+ @Override public Number getFlagValue() { return null; }
@Override public void setProperty(Object description, Object property) {}
@Override public void removeProperty(Object description) {}
@Override public Object getProperty(Object description) { return null; }
View
11 src/main/org/openscience/cdk/geometry/cip/ImmutableHydrogen.java
@@ -437,6 +437,17 @@ public boolean getFlag(int flagType) {
return null;
}
+
+ /**
+ * This field is not used by this immutable hydrogen.
+ *
+ * @return null.
+ */
+ @Override
+ public Number getFlagValue() {
+ return null;
+ }
+
/** {@inheritDoc}} */
/**
* This field is not used by this immutable hydrogen.
View
66 src/main/org/openscience/cdk/interfaces/IChemObject.java
@@ -153,23 +153,42 @@
public void setID(String identifier);
/**
- * Sets the value of some flag.
- *
- * @param flag_type Flag to set
- * @param flag_value Value to assign to flag
- * @see #getFlag
+ * Sets the value of some flag. The flag is a mask from a given
+ * CDKConstant (e.g. {@link org.openscience.cdk.CDKConstants#ISAROMATIC}
+ * or {@link org.openscience.cdk.CDKConstants#VISITED}).
+ *
+ * <pre>{@code
+ * // set this chem object to be aromatic
+ * chemObject.setFlag(CDKConstants.ISAROMATIC, Boolean.TRUE);
+ * // ...
+ * // indicate we have visited this chem object
+ * chemObject.setFlag(CDKConstants.VISITED, Boolean.TRUE);
+ * }</pre>
+ *
+ * @param mask flag to set the value for
+ * @param value value to assign to flag
+ * @see #getFlag
+ * @see org.openscience.cdk.CDKConstants
*/
- public void setFlag(int flag_type, boolean flag_value);
+ public void setFlag(int mask, boolean value);
/**
- * Returns the value of some flag.
+ * Returns the value of a given flag. The flag is a mask from a given
+ * CDKConstant (e.g. {@link org.openscience.cdk.CDKConstants#ISAROMATIC}).
+ *
+ * <pre>{@code
+ * if(chemObject.getFlag(CDKConstants.ISAROMATIC)){
+ * // handle aromatic flag on this chem object
+ * }
+ * }</pre>
*
- * @param flag_type Flag to retrieve the value of
- * @return true if the flag <code>flag_type</code> is set
- * @see #setFlag
+ * @param mask flag to retrieve the value of
+ * @return true if the flag <code>flag_type</code> is set
+ * @see #setFlag
+ * @see org.openscience.cdk.CDKConstants
*/
- public boolean getFlag(int flag_type);
+ public boolean getFlag(int mask);
/**
* Sets the properties of this object.
@@ -180,22 +199,39 @@
public void setProperties(Map<Object,Object> properties);
/**
- * Sets the whole set of flags.
+ * Sets the whole set of flags. This set will iteratively invoke
+ * {@link #setFlag(int, boolean)} for each value in the array and
+ * use {@link org.openscience.cdk.CDKConstants#FLAG_MASKS} to look
+ * up the correct mask. If only a single flag is being set it is a lot
+ * faster to use {@link #setFlag(int, boolean)}.
*
- * @param flagsNew the new flags.
+ * @param newFlags the new flags to set.
+ * @see #setFlag(int, boolean)
* @see #getFlags
*/
- public void setFlags(boolean[] flagsNew);
+ public void setFlags(boolean[] newFlags);
/**
- * Returns the whole set of flags.
+ * Returns the whole set of flags. This method will create a new array on
+ * each invocation and it is recommend you use {@link #getFlagValue()}
+ * if you need all the flags. For individual flags please use {@link #getFlag(int)}
*
* @return the flags.
* @see #setFlags
+ * @see #getFlag(int)
+ * @see #getFlagValue()
*/
public boolean[] getFlags();
/**
+ * Access the internal value used to store the flags. The flags are stored
+ * on a single numeric value and are set/cleared.
+ *
+ * @return numeric representation of the flags
+ */
+ public Number getFlagValue();
+
+ /**
* Returns a one line description of this IChemObject.
*
* @return a String representation of this object
View
64 src/main/org/openscience/cdk/isomorphism/matchers/QueryChemObject.java
@@ -28,6 +28,7 @@
import org.openscience.cdk.CDKConstants;
import org.openscience.cdk.interfaces.IAtom;
+import org.openscience.cdk.interfaces.IChemObject;
import org.openscience.cdk.interfaces.IChemObjectBuilder;
import org.openscience.cdk.interfaces.IChemObjectChangeEvent;
import org.openscience.cdk.interfaces.IChemObjectListener;
@@ -36,7 +37,7 @@
* @cdk.module isomorphism
* @cdk.githash
*/
-public class QueryChemObject {
+public class QueryChemObject implements IChemObject {
/**
* List for listener administration.
@@ -61,10 +62,9 @@
* flag array with self-defined constants (flags[VISITED] = true). 100 flags
* per object should be more than enough.
*/
- private boolean[] flags;
+ private short flags; // flags are currently stored as a single short value MAX_FLAG_INDEX < 16
public QueryChemObject() {
- flags = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
chemObjectListeners = null;
properties = null;
identifier = null;
@@ -272,28 +272,26 @@ public void setID(String identifier)
}
/**
- * Sets the value of some flag.
- *
- *@param flag_type Flag to set
- *@param flag_value Value to assign to flag
- *@see #getFlag
+ * @inheritDoc
*/
- public void setFlag(int flag_type, boolean flag_value)
+ @Override
+ public void setFlag(int mask, boolean value)
{
- flags[flag_type] = flag_value;
+ // set/unset a bit in the flags value
+ if (value)
+ flags |= mask;
+ else
+ flags &= ~(mask);
notifyChanged();
}
/**
- * Returns the value of some flag.
- *
- *@param flag_type Flag to retrieve the value of
- *@return true if the flag <code>flag_type</code> is set
- *@see #setFlag
+ * @inheritDoc
*/
- public boolean getFlag(int flag_type)
+ @Override
+ public boolean getFlag(int mask)
{
- return flags[flag_type];
+ return (flags & mask) != 0;
}
/**
@@ -316,23 +314,35 @@ public void setProperties(Map<Object,Object> properties)
private boolean doNotification = true;
/**
- * Sets the whole set of flags.
- *
- * @param flagsNew the new flags.
- * @see #getFlags
+ * @inheritDoc
*/
+ @Override
public void setFlags(boolean[] flagsNew){
- flags=flagsNew;
+ for(int i = 0; i < flagsNew.length ; i++)
+ setFlag(CDKConstants.FLAG_MASKS[i], flagsNew[i]);
}
/**
- * Returns the whole set of flags.
- *
- *@return the flags.
- *@see #setFlags
+ * @inheritDoc
*/
+ @Override
public boolean[] getFlags(){
- return(flags);
+ // could use a list a invoke .toArray() on the return
+ boolean[] flagArray = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
+ for(int i = 0 ; i < CDKConstants.FLAG_MASKS.length; i++){
+ int mask = CDKConstants.FLAG_MASKS[i];
+ flagArray[i] = getFlag(mask);
+ }
+ return flagArray;
+ }
+
+
+ /**
+ * @inheritDoc
+ */
+ @Override
+ public Short getFlagValue(){
+ return flags;
}
public void setNotification(boolean bool) {
View
97 src/main/org/openscience/cdk/silent/ChemObject.java
@@ -24,9 +24,12 @@
package org.openscience.cdk.silent;
import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
+import java.util.List;
import java.util.Map;
import org.openscience.cdk.CDKConstants;
@@ -69,7 +72,7 @@
* flag array with self-defined constants (flags[VISITED] = true). 100 flags
* per object should be more than enough.
*/
- private boolean[] flags;
+ private short flags; // flags are currently stored as a single short value MAX_FLAG_INDEX < 16
/**
* The ID is null by default.
@@ -82,7 +85,6 @@
*/
public ChemObject()
{
- flags = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
properties = null;
identifier = null;
}
@@ -90,17 +92,15 @@ public ChemObject()
/**
* Constructs a new IChemObject by copying the flags, and the
* identifier. It does not copy the listeners and properties.
- *
+ *
* @param chemObject the object to copy
*/
public ChemObject(IChemObject chemObject) {
// copy the flags
- boolean[] oldflags = chemObject.getFlags();
- flags = new boolean[oldflags.length];
- System.arraycopy(oldflags, 0, flags, 0, flags.length);
+ flags = chemObject.getFlagValue().shortValue();
// copy the identifier
identifier = chemObject.getID();
- }
+ }
/**
* Use this to add yourself to this IChemObject as a listener. In order to do
@@ -235,8 +235,8 @@ public Object clone() throws CloneNotSupportedException
{
ChemObject clone = (ChemObject)super.clone();
// clone the flags
- clone.flags = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
- System.arraycopy(flags, 0, clone.flags, 0, flags.length);
+ clone.flags = this.getFlagValue();
+
// clone the properties
if (properties != null) {
Map<Object, Object> clonedHashtable = new HashMap<Object, Object>();
@@ -265,7 +265,7 @@ public boolean compare(Object object)
return false;
}
ChemObject chemObj = (ChemObject) object;
- return identifier == chemObj.identifier;
+ return identifier == chemObj.identifier;
}
@@ -294,30 +294,36 @@ public void setID(String identifier)
/**
- * Sets the value of some flag.
- *
- *@param flag_type Flag to set
- *@param flag_value Value to assign to flag
- *@see #getFlag
+ * @inheritDoc
*/
- public void setFlag(int flag_type, boolean flag_value)
+ @Override
+ public void setFlag(int mask, boolean value)
{
- flags[flag_type] = flag_value;
+ // set/unset a bit in the flags value
+ if (value)
+ flags |= mask;
+ else
+ flags &= ~(mask);
+
}
- /**
- * Returns the value of some flag.
- *
- *@param flag_type Flag to retrieve the value of
- *@return true if the flag <code>flag_type</code> is set
- *@see #setFlag
- */
- public boolean getFlag(int flag_type)
+ /**
+ * @inheritDoc
+ */
+ @Override
+ public boolean getFlag(int mask)
{
- return flags[flag_type];
+ return (flags & mask) != 0;
}
+ /**
+ * @inheritDoc
+ */
+ public Short getFlagValue(){
+ return flags; // auto-boxing
+ }
+
/**
* Sets the properties of this object.
@@ -334,26 +340,29 @@ public void setProperties(Map<Object,Object> properties)
lazyProperties().put(key, properties.get(key));
}
}
-
-
- /**
- * Sets the whole set of flags.
- *
- * @param flagsNew the new flags.
- * @see #getFlags
- */
+
+
+ /**
+ * @inheritDoc
+ */
+ @Override
public void setFlags(boolean[] flagsNew){
- flags=flagsNew;
+ for(int i = 0; i < flagsNew.length ; i++)
+ setFlag(CDKConstants.FLAG_MASKS[i], flagsNew[i]);
}
- /**
- * Returns the whole set of flags.
- *
- *@return the flags.
- *@see #setFlags
- */
+ /**
+ * @inheritDoc
+ */
+ @Override
public boolean[] getFlags(){
- return(flags);
+ // could use a list a invoke .toArray() on the return
+ boolean[] flagArray = new boolean[CDKConstants.MAX_FLAG_INDEX + 1];
+ for(int i = 0 ; i < CDKConstants.FLAG_MASKS.length; i++){
+ int mask = CDKConstants.FLAG_MASKS[i];
+ flagArray[i] = getFlag(mask);
+ }
+ return flagArray;
}
/**
@@ -372,13 +381,13 @@ public Object shallowCopy()
}
return copy;
}
-
+
public IChemObjectBuilder getBuilder() {
return SilentChemObjectBuilder.getInstance();
}
private boolean doNotification = true;
-
+
public void setNotification(boolean bool) {
this.doNotification = bool;
}
View
1  src/test/org/openscience/cdk/interfaces/AbstractChemObjectTest.java
@@ -104,7 +104,6 @@
}
@Test public void testGetFlags(){
- System.out.println("Test flags:");
IChemObject chemObject=newChemObject();
chemObject.setFlag(CDKConstants.ISINRING,true);
IChemObject chemObject2=newChemObject();

0 comments on commit 1a1b03d

Please sign in to comment.
Something went wrong with that request. Please try again.