Thus spake Flint1b:
I have made a rough proposal now under PR #75
github.com/vassalengine/vassal/pull/75[1], can someone, maybe
Brent, have a look at this and give me some feedback? This downcasting
sure looks unsafe but if we are 100% sure that only objects of the
proper type ever pass through this code then casting like this is ok.
Of possible relevance here is what’s in the VASSAL.property package.
It contains some machinery for getting and setting properties in a
type-safe way which I added for 3.2 as a start for cleaning up property
handling, and then it never got used.
The two interfaces PropertyExporter extends, PropertyNameSource and
PropertySource, are from just before I started introducing generics,
and are really unfortunate in that they keys and values are Objects
despite that we do know their types…
The VASSAL.property package was intended to address this. There’s
a lot of simplification to be had if it, or something like it were
to be used (and everything converted to use it). There’s also support
for separating persistent state from ephemeral state, which could be
some help toward effecting model-view separation.
Also:
“uckelman” wrote:
What I had was a top-level piece wrapper containing a HashMap from
property names to getters and setters for the properties. What this
does is change getting or setting a property from being O(n) to O(1),
which should make a massive difference with pieces that have a long
list of traits. (It might be the single most significant optimization
we could do for a lot of modules.)
I still have it somewhere. I should dig that out.
You haven’t used reflection for this have you? It’s ok to look at the
method names and deduce the property names based on JavaBean convention
(getX setX isX etc) but most other kinds of reflection are evil.
And either way, can you dig that out if you find the time?
I had a look around and turned up a few of my benchmark cases, but not
the code I had in mind. No matter, as I can still describe it:
Every trait class has a pair of getProperty() and setProperty() methods, which are more or less similar structurally. Here are the ones from Decorator:
@Override
public Object getProperty(Object key) {
if (Properties.KEY_COMMANDS.equals(key)) {
return getKeyCommands();
}
else if (Properties.INNER.equals(key)) {
return piece;
}
else if (Properties.OUTER.equals(key)) {
return dec;
}
else if (Properties.VISIBLE_STATE.equals(key)) {
return myGetState()+piece.getProperty(key);
}
else if (Properties.SELECTED.equals(key)) {
return selected;
}
else {
return piece.getProperty(key);
}
}
@Override
public void setProperty(Object key, Object val) {
if (Properties.INNER.equals(key)) {
setInner((GamePiece) val);
}
else if (Properties.OUTER.equals(key)) {
dec = (Decorator) val;
}
/**
* Cache Selection status and pass it on to all inner traits.
*/
else if (Properties.SELECTED.equals(key)) {
if (val instanceof Boolean) {
setSelected((Boolean) val);
}
else {
setSelected(false);
}
piece.setProperty(key, val);
}
else {
piece.setProperty(key, val);
}
}
These are painfully slow. Whenever you want to do a lookup of a
property, you havae to do a number of string comparisons which is O(n)
in the number of total properties—and not just the total number of
properties in the trait where your property is, but the total number of
properties in the whole stack of traits. This is tollerable when you
have simple pieces, but becomes a disaster when you have pieces with 50
traits (as could be seen in one of Brian’s stack traces a week or two
ago). If you’re unlucky and the property you want is in a trait near the
bottom of the stack, you will have to do hundreds of string comparisons
to get that value. The Gang of Four book should have included a note
that the Decorator pattern was not suitable for stacks of decorators as
deep as we have.
There are at least two ways the above could be improved: Locally, and globally.
Locally, one could put getters and setters for a trait into a map and
have O(1) access to properties within each trait:
protected interface Getter {
public Object get(GamePiece p, Object key);
}
protected interface Setter {
public void set(GamePiece p, Object key, Object value);
}
// This is a map of containing the getting code from old getProperty()
protected static final Map<Object, Getter> getters = Map.ofEntries<>(
entry(Properties.KEY_COMMANDS, (p, k) → p.getKeyCommands()),
entry(Properties.INNER, (p, k) → p.piece),
entry(Properties.OUTER, (p, k) → p.dec),
entry(Properties.VISIBLE_STATE, (p, k) →
p.myGetState() + p.piece.getProperty(k)),
entry(Properties.SELECTED, (p, k) → p.selected)
);
// This is a map of containing the setting code from old setProperty()
protected static final Map<Object, Setter> setters = Map.ofEntries<>(
entry(Properties.INNER, (p, k, v) → p.setInner((GamePiece) v)),
entry(Properties.OUTER, (p, k, v) → p.dec = (Decorator) v),
entry(Properties.SELECTED, (p, k, v) → {
setSelected(v instanceof Boolean ? (Boolean) v : false);
p.piece.setProperty(k, v);
})
);
// … or one could have a Map<Object, Pair<Getter, Setter>> to avoid
// repetition
// These cover the extremal case, when the decorator delegates getting or
// setting to the piece it wraps
protected static final Getter delegatedGetter =
(p, k) → p.piece.getProperty(k);
protected static final Setter delegatedSetter =
(p, k, v) → p.piece.setProperty(k, v);
@Override
public Object getProperty(Object key) {
return getters.getOrDefault(key, delegatedGetter).get(this, key);
}
@Override
public void setProperty(Object key, Object value) {
return setters.getOrDefault(key, delegatedSetter).set(this, key, value);
}
This ought to be quite a bit faster for property lookup. You still have
to traverse the chain of traits, but each step will be faster, since you
can avoid doing so many string comparisons at each step. (If you have a
stack of 20 traits, each with 9 properties and you have to go all the
way to the bottom to retrieve a property, it’s about 2.5x faster. It’s
still O(n), but now n is the number of traits rather than the number of
properties, so n is much smaller.) This also has the virtue of being
invisible from the outside.
The best solution, however, would be to store the outermost getter and
setter for each propery in a map held by the outermost decorator. That
way, there would be completely flat constant-time access to all
properties. This isn’t as straightforward to do as what I described
above, as it’s not static—it depends on trait order, so would have to
be set up for each piece (or, if one wanted to be clever, piece type).
Basically what you’d do at creation time is traverse the trait chain and
grab the first getter/setter you find for each property as you walk
toward the end. It may be possible to manage this without breaking the
API as well, though doing so could be rather fiddly. I know of nothing
which would improve performance across all modules more than this
change.
–
J.
