12:47 a.m.
[First post. I apologize in advance if I'm following the wrong protocol to
report a bug.]
I had a rule using the collectSet() accumulator that was giving me an NPE at
line 123 of CollectSetAccumulateFunction.java, highlighted below:
public void reverse(Serializable context,
Object value) throws Exception {
CollectListData data = (CollectListData) context;
CollectListData.MutableInt counter = data.map.get( value );
*/if( (--counter.value) == 0 ) {/*
data.map.remove( value );
}
}
When looking in the debugger, I could see that the "value" passed in was
indeed present within the "data.map". However, the
"data.map.get(value)" was
returning null. I thankfully realized this was because the object had
changed, which changed its hashCode(). The stored hashCode() in the HashMap
was different from its current hashCode().
I solved my problem by reimplementing my hashCode() as
System.identityHashCode(this) and my equals as "this == other".
However, I suspect the CollectSetAccumulateFunction should really use an
IdentityHashMap in this situation. It's a really bad idea to allow mutable
objects as keys in HashMap, and there is nothing about the collectSet()
accumulator function that suggests that it should only collect immutable
objects.
Thanks.
P.S. I'm loving Drools. Great tool.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
7:46 a.m.
I don't think that this would be a good idea. Consider, for instance,
IdentityHashMap<String,Integer> map =
new IdentityHashMap<String,Integer>();
map.put( "one", 1 );
map.put( new String( "one" ), 2 );
which results in kay and value sets:
keys: [one, one]
values: [2, 1]
"equals" should not be based on mutable properties reflecting state,
just like a body paint job doesn't change the identity of your car -
it just makes the Joneses think so ;-)
-W
On 25/02/2012, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
[First post. I apologize in advance if I'm following the wrong
protocol to
report a bug.]
I had a rule using the collectSet() accumulator that was giving me an NPE at
line 123 of CollectSetAccumulateFunction.java, highlighted below:
public void reverse(Serializable context,
Object value) throws Exception {
CollectListData data = (CollectListData) context;
CollectListData.MutableInt counter = data.map.get( value );
*/if( (--counter.value) == 0 ) {/*
data.map.remove( value );
}
}
When looking in the debugger, I could see that the "value" passed in was
indeed present within the "data.map". However, the
"data.map.get(value)" was
returning null. I thankfully realized this was because the object had
changed, which changed its hashCode(). The stored hashCode() in the HashMap
was different from its current hashCode().
I solved my problem by reimplementing my hashCode() as
System.identityHashCode(this) and my equals as "this == other".
However, I suspect the CollectSetAccumulateFunction should really use an
IdentityHashMap in this situation. It's a really bad idea to allow mutable
objects as keys in HashMap, and there is nothing about the collectSet()
accumulator function that suggests that it should only collect immutable
objects.
Thanks.
P.S. I'm loving Drools. Great tool.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
8:28 p.m.
I'm not really trying to make an abstract point that all maps should be
identity maps. The problem I encountered was very subtle (it took me a day
to track it down), and it's difficult to see how to resolve it without
resorting to identity semantics somewhere in the accumulator implementation.
Consider a very basic rule using an accumulator (and a class called Fooble
with an isRelevant() accessor):
rule "Test sets"
when
accumulate(
$o : Fooble(relevant)
$oSet : collectSet( $o ) )
then
...
end
The general principle behind the rules engine is that whenever something
known to the engine changes such that it affects one of the conditions in a
rule, the rule will reevaluate. So, if there were three "relevant" Foobles,
the set would contain them (possibly, I now confess, holding less than three
objects, if any of them are equals() to another). If later, there were only
two relevant Foobles, one would expect the set to contain those two.
The great optimization of the rule engine is that the reevaluations are done
for only those portions of the underlying RETE structure that are affected.
The accumulator functions support this by the fact that they maintain state
and have accumulate() and reverse() functions.
But, because of the particular choices made in the implementation of
CollectSetAccumulateFunction(), it failed, in my case, to reverse() an
object that was previously relevant, because that object had a different
hashCode()/equals() when the time came to reverse. And the failure was such
that an NPE was involved, and ignoring the NPE (or ignoring the fact that
the reversed object wasn't found in the set) would have caused the set to
still contain the object it was supposed to remove.
On the other hand, if an IdentityHashMap were used in place of the HashMap,
it would have been able to reverse() the object, regardless of its equals().
There would be one other change required, mostly likely. The getResults()
would probably have to change from...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( data.map.keySet() );
}
...to...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( new HashSet(data.map.keySet())
);
}
That would take the identity-based keySet (being the accumulated objects)
and collect them into an equals()-based Set. That would be, unfortunately,
an O(N) operation inserted into a function that is otherwise an O(1). But,
it would preserve the current functionality of collectSet() without failing
in the case of objects that are not suitable as keys.
Alternatively, the set returned by collectSet() could changed to be an
identity-based Set, which would need to be called out in release notes,
naturally.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
8:41 p.m.
By the way, while checking into various scenarios prompted by Wolfgang's
response, I came up with a very interesting test program:
rule "Initialize"
salience 100
when
eval(true)
then
insert("one");
insert(new String("one"));
insertLogical("two");
insertLogical(new String("two"));
end
rule "Find strings"
salience 50
when
$s : String()
then
System.out.println("I found string: " + $s);
end
rule "Gather strings"
salience 50
when
accumulate(
$s : String(),
$sSet : collectSet( $s ),
$sList : collectList( $s ) )
then
System.out.println("Here is the set of all strings: " + $sSet);
System.out.println("Here is the list of all strings: " + $sList);
end
What would you expect that to output? How many "I found string: xxx" lines
would there be? How many entries in the "set of all strings"? How many
entries in the "list of all strings"?
For my part, I figured there would be four "I found string: xxx" lines, two
entries in the set, and four entries in the list.
I was very surprised to find the following output:
I found string: two
I found string: one
I found string: one
Here is the set of all strings: [two, one]
Here is the list of all strings: [one, one, two]
This strongly suggests that "insert" objects are held in an
identity-preserving structure, but that "insertLogical" objects are held in
an equals()-based structure.
I am glad for the identity-based handling of "insert" objects, especially
considering that all of my inserted objects come from "outside" the engine.
I would hate to see one of them fail to be considered by the rule engine.
I can almost see some usefulness for the equals() behavior of insertLogical
objects, but far more potential problems.
I think it's slightly dangerous that these behaviors differ and that it is
not, AFAIK, documented in a way that is clear to users of Drools.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
1:24 a.m.
accumulate supports custom functions, just add your own version of
collectSet that uses IdentityMap.
Mark
On 25/02/2012 19:28, SirMungus wrote:
I'm not really trying to make an abstract point that all maps
should be
identity maps. The problem I encountered was very subtle (it took me a day
to track it down), and it's difficult to see how to resolve it without
resorting to identity semantics somewhere in the accumulator implementation.
Consider a very basic rule using an accumulator (and a class called Fooble
with an isRelevant() accessor):
rule "Test sets"
when
accumulate(
$o : Fooble(relevant)
$oSet : collectSet( $o ) )
then
...
end
The general principle behind the rules engine is that whenever something
known to the engine changes such that it affects one of the conditions in a
rule, the rule will reevaluate. So, if there were three "relevant" Foobles,
the set would contain them (possibly, I now confess, holding less than three
objects, if any of them are equals() to another). If later, there were only
two relevant Foobles, one would expect the set to contain those two.
The great optimization of the rule engine is that the reevaluations are done
for only those portions of the underlying RETE structure that are affected.
The accumulator functions support this by the fact that they maintain state
and have accumulate() and reverse() functions.
But, because of the particular choices made in the implementation of
CollectSetAccumulateFunction(), it failed, in my case, to reverse() an
object that was previously relevant, because that object had a different
hashCode()/equals() when the time came to reverse. And the failure was such
that an NPE was involved, and ignoring the NPE (or ignoring the fact that
the reversed object wasn't found in the set) would have caused the set to
still contain the object it was supposed to remove.
On the other hand, if an IdentityHashMap were used in place of the HashMap,
it would have been able to reverse() the object, regardless of its equals().
There would be one other change required, mostly likely. The getResults()
would probably have to change from...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( data.map.keySet() );
}
...to...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( new HashSet(data.map.keySet())
);
}
That would take the identity-based keySet (being the accumulated objects)
and collect them into an equals()-based Set. That would be, unfortunately,
an O(N) operation inserted into a function that is otherwise an O(1). But,
it would preserve the current functionality of collectSet() without failing
in the case of objects that are not suitable as keys.
Alternatively, the set returned by collectSet() could changed to be an
identity-based Set, which would need to be called out in release notes,
naturally.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
2:05 a.m.
There are two types of facts - stated and justified. As explained in the
TMS section.
Justified facts are ALWAYS equality based, as it's the nature of TMS.
The other part of engine for stated facts is customizable and may work
in either equality mode or identity mode. Identity mode is default, as
it's the most common way java developers expect to work, knowledge
based engineers may prefer eqality mode.
We do discuss this in the maual. If it's clear, then please do help beef
up the docs on this. Fork the docs and submit a pull request, see:
http://www.athico.com/Getting_Involved/gettinginvolved.html
Mark
On 25/02/2012 19:41, SirMungus wrote:
By the way, while checking into various scenarios prompted by
Wolfgang's
response, I came up with a very interesting test program:
rule "Initialize"
salience 100
when
eval(true)
then
insert("one");
insert(new String("one"));
insertLogical("two");
insertLogical(new String("two"));
end
rule "Find strings"
salience 50
when
$s : String()
then
System.out.println("I found string: " + $s);
end
rule "Gather strings"
salience 50
when
accumulate(
$s : String(),
$sSet : collectSet( $s ),
$sList : collectList( $s ) )
then
System.out.println("Here is the set of all strings: " + $sSet);
System.out.println("Here is the list of all strings: " + $sList);
end
What would you expect that to output? How many "I found string: xxx" lines
would there be? How many entries in the "set of all strings"? How many
entries in the "list of all strings"?
For my part, I figured there would be four "I found string: xxx" lines, two
entries in the set, and four entries in the list.
I was very surprised to find the following output:
I found string: two
I found string: one
I found string: one
Here is the set of all strings: [two, one]
Here is the list of all strings: [one, one, two]
This strongly suggests that "insert" objects are held in an
identity-preserving structure, but that "insertLogical" objects are held in
an equals()-based structure.
I am glad for the identity-based handling of "insert" objects, especially
considering that all of my inserted objects come from "outside" the engine.
I would hate to see one of them fail to be considered by the rule engine.
I can almost see some usefulness for the equals() behavior of insertLogical
objects, but far more potential problems.
I think it's slightly dangerous that these behaviors differ and that it is
not, AFAIK, documented in a way that is clear to users of Drools.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
6:44 p.m.
Perhaps I should elaborate what I was hinting at with my joke ("Joneses").
A Java object (pojo) can be duplicated so that the new object will not
have the same identity (as tested by "=="). Its equals() method will return
true if applied to this pair of pojos. This method may still return true
after changing
certain properties, as it is generally customary to make the assumption that
some properties (primary key) uniquely identify the real world entity a
pojo
represents. Also note that applications are usually based on a 1:1
relationship
between entities and pojos.
When we insert pojos as facts we have to contend with the notion that
facts represent truths. It does not make sense to insert a clone (i.e,
another
pojo instance with all properties being the same) of a fact - it can't be
truer
than true. But it *might* make sense to insert another object relating to
the
same real world entity, i.e., Entity.equals() returning true, but not
*all*properties
being equal, for instance two observations of the same thing, at different
times or by different observers.
In other words, an equals() that works well for testing the identity of
entities
represented by pojos is usually not the same as an equals() testing the
sameness of a fact.
-W
On 26/02/2012, Mark Proctor <mproctor(a)codehaus.org> wrote:
There are two types of facts - stated and justified. As explained in
the
TMS section.
Justified facts are ALWAYS equality based, as it's the nature of TMS.
The other part of engine for stated facts is customizable and may work
in either equality mode or identity mode. Identity mode is default, as
it's the most common way java developers expect to work, knowledge
based engineers may prefer eqality mode.
We do discuss this in the maual. If it's clear, then please do help beef
up the docs on this. Fork the docs and submit a pull request, see:
http://www.athico.com/Getting_Involved/gettinginvolved.html
Mark
On 25/02/2012 19:41, SirMungus wrote:
> By the way, while checking into various scenarios prompted by Wolfgang's
> response, I came up with a very interesting test program:
>
> rule "Initialize"
> salience 100
> when
> eval(true)
> then
> insert("one");
> insert(new String("one"));
> insertLogical("two");
> insertLogical(new String("two"));
> end
>
> rule "Find strings"
> salience 50
> when
> $s : String()
> then
> System.out.println("I found string: " + $s);
> end
>
> rule "Gather strings"
> salience 50
> when
> accumulate(
> $s : String(),
> $sSet : collectSet( $s ),
> $sList : collectList( $s ) )
> then
> System.out.println("Here is the set of all strings: " + $sSet);
> System.out.println("Here is the list of all strings: " +
$sList);
> end
>
> What would you expect that to output? How many "I found string: xxx"
lines
> would there be? How many entries in the "set of all
strings"? How many
> entries in the "list of all strings"?
>
> For my part, I figured there would be four "I found string: xxx" lines,
> two
> entries in the set, and four entries in the list.
>
> I was very surprised to find the following output:
>
> I found string: two
> I found string: one
> I found string: one
> Here is the set of all strings: [two, one]
> Here is the list of all strings: [one, one, two]
>
> This strongly suggests that "insert" objects are held in an
> identity-preserving structure, but that "insertLogical" objects are held
> in
> an equals()-based structure.
>
> I am glad for the identity-based handling of "insert" objects, especially
> considering that all of my inserted objects come from "outside" the
> engine.
> I would hate to see one of them fail to be considered by the rule engine.
>
> I can almost see some usefulness for the equals() behavior of
> insertLogical
> objects, but far more potential problems.
>
> I think it's slightly dangerous that these behaviors differ and that it
is
> not, AFAIK, documented in a way that is clear to users of
Drools.
>
> --
> View this message in context:
>
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
> Sent from the Drools: Developer (committer) mailing list mailing
list
> archive at Nabble.com.
> _______________________________________________
> rules-dev mailing list
> rules-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
4:47 p.m.
Mark Proctor wrote
There are two types of facts - stated and justified. As explained in the
TMS section.
Justified facts are ALWAYS equality based, as it's the nature of TMS.
The other part of engine for stated facts is customizable and may work
in either equality mode or identity mode. Identity mode is default, as
it's the most common way java developers expect to work, knowledge
based engineers may prefer eqality mode.
We do discuss this in the maual.
Yes, I see that, and it makes sense.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
5:53 p.m.
Mark Proctor wrote
accumulate supports custom functions, just add your own version of
collectSet that uses IdentityMap.
I could do that, but please let me take one more stab, just in case I
haven't been clear enough.
1) I am not suggesting that collectSet() return an identity-based set. I am
content to have it return a normal set, though I had early confusion on that
point.
2) The discussion above about stated vs. justified facts turns out to be a
distraction to the main point of the bug that I believe to be in
CollectSetAccumulateFunction. My failure to understand the distinction is
not germane to the bug.
3) I am not disagreeing with the value behind implementing equals() to only
take into account invariant fields, though I am stating that goes beyond the
explicity documentation of equals()/hashCode() and is explicitly *not*
followed by many classes in the JRE (most notably, collections).
There is nothing about the purpose of collectSet() that logically suggests
that it should only work with objects that have invariant
equals()/hashCode() functions. If five objects match a rule, they should be
able to be collected into a set (which may end up with fewer than five
objects). If things change such that only three facts match the rule, they
should still be able to be collected into a set. There's no compelling
reason why that should only apply to objects with invariant equals(). And it
seems to me that you rarely want to leave an NPE in code, rather than
catching it and throwing a more intelligible exception (e.g., "Object to be
removed from collectSet() was not found").
If the internal maintenance of state in CollectSetAccumulateFunction used
identity semantics and applied the equality semantics only when you asked it
for its results, it would maintain the same external behavior (returning a
"normal" equality-based set) without failing to work for objects with
variant equals.
If you Google around for the proper implementation of equals(), you will
find many serious sites that discuss it at length, yet provide examples
like:
public boolean equals(Object aThat){
if ( this == aThat ) return true;
if ( !(aThat instanceof Car) ) return false;
Car that = (Car)aThat;
return
EqualsUtil.areEqual(this.fName, that.fName) &&
EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors) &&
EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage) &&
EqualsUtil.areEqual(this.fColor, that.fColor) &&
Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
//array!
}
Ironically, that is explicitly against Wolfgang's suggestion above!
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
8 p.m.
“When *I* use a word,” Humpty Dumpty said, in rather a scornful tone,
“it means just what I choose it to mean—neither more nor less.”
“The question is,” said Alice, “whether you *can* make words mean so
many different things.”
“The question is,” said Humpty Dumpty, “which is to be master that’s
all.”
If a fact represents an entity with an immutable property set identifying
it and a mutable set of state properties I define equals() to mean "is the
same entity as".
If a fact represents a proposition I require equals() - taking into account
all properties - to mean "declares the same as", and uphold the notion that
such a fact shouldn't be modified at all, just retracted.
You can collect java.util.Sets of both, but don't violate the respective
semantics.
And here's a quote from java.util.Set's javadoc: Note: Great care must be
exercised if mutable objects are used as set elements. The behavior of a
set is not specified if the value of an object is changed in a manner that
affects equals comparisons while the object is an element in the set.
I'd say that this caveat extends to the usage of collectSet().
Of course, properly catching NPEs - even if they result from abuse - should
be done whenever appropriate.
-W
On 27 February 2012 17:53, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
Mark Proctor wrote
>
> accumulate supports custom functions, just add your own version of
> collectSet that uses IdentityMap.
>
I could do that, but please let me take one more stab, just in case I
haven't been clear enough.
1) I am not suggesting that collectSet() return an identity-based set. I am
content to have it return a normal set, though I had early confusion on
that
point.
2) The discussion above about stated vs. justified facts turns out to be a
distraction to the main point of the bug that I believe to be in
CollectSetAccumulateFunction. My failure to understand the distinction is
not germane to the bug.
3) I am not disagreeing with the value behind implementing equals() to only
take into account invariant fields, though I am stating that goes beyond
the
explicity documentation of equals()/hashCode() and is explicitly *not*
followed by many classes in the JRE (most notably, collections).
There is nothing about the purpose of collectSet() that logically suggests
that it should only work with objects that have invariant
equals()/hashCode() functions. If five objects match a rule, they should be
able to be collected into a set (which may end up with fewer than five
objects). If things change such that only three facts match the rule, they
should still be able to be collected into a set. There's no compelling
reason why that should only apply to objects with invariant equals(). And
it
seems to me that you rarely want to leave an NPE in code, rather than
catching it and throwing a more intelligible exception (e.g., "Object to be
removed from collectSet() was not found").
If the internal maintenance of state in CollectSetAccumulateFunction used
identity semantics and applied the equality semantics only when you asked
it
for its results, it would maintain the same external behavior (returning a
"normal" equality-based set) without failing to work for objects with
variant equals.
If you Google around for the proper implementation of equals(), you will
find many serious sites that discuss it at length, yet provide examples
like:
public boolean equals(Object aThat){
if ( this == aThat ) return true;
if ( !(aThat instanceof Car) ) return false;
Car that = (Car)aThat;
return
EqualsUtil.areEqual(this.fName, that.fName) &&
EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors) &&
EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage) &&
EqualsUtil.areEqual(this.fColor, that.fColor) &&
Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
//array!
}
Ironically, that is explicitly against Wolfgang's suggestion above!
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
8:25 a.m.
The getResults()
would probably have to change from...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( data.map.keySet() );
}
...to...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( new HashSet(data.map.keySet())
);
}
The proposed getResult() results in a full HashSet copy for each change of Rete, that
performance hit would be unbearable for most people.
I do agree that there is some merit to what you are asking, but we can't give that
perf hit to everyone. It would be better to maintain two collections, one identity and one
set based. And some how only change in set what was relevant to the change in identity,
you might need to build a custom hash table for this - see AbstractHashTable in drools
which can easily be extended for custom keys and custom add/remove methods.
Mark
On 27/02/2012 16:53, SirMungus wrote:
Mark Proctor wrote
> accumulate supports custom functions, just add your own version of
> collectSet that uses IdentityMap.
>
I could do that, but please let me take one more stab, just in case I
haven't been clear enough.
1) I am not suggesting that collectSet() return an identity-based set. I am
content to have it return a normal set, though I had early confusion on that
point.
2) The discussion above about stated vs. justified facts turns out to be a
distraction to the main point of the bug that I believe to be in
CollectSetAccumulateFunction. My failure to understand the distinction is
not germane to the bug.
3) I am not disagreeing with the value behind implementing equals() to only
take into account invariant fields, though I am stating that goes beyond the
explicity documentation of equals()/hashCode() and is explicitly *not*
followed by many classes in the JRE (most notably, collections).
There is nothing about the purpose of collectSet() that logically suggests
that it should only work with objects that have invariant
equals()/hashCode() functions. If five objects match a rule, they should be
able to be collected into a set (which may end up with fewer than five
objects). If things change such that only three facts match the rule, they
should still be able to be collected into a set. There's no compelling
reason why that should only apply to objects with invariant equals(). And it
seems to me that you rarely want to leave an NPE in code, rather than
catching it and throwing a more intelligible exception (e.g., "Object to be
removed from collectSet() was not found").
If the internal maintenance of state in CollectSetAccumulateFunction used
identity semantics and applied the equality semantics only when you asked it
for its results, it would maintain the same external behavior (returning a
"normal" equality-based set) without failing to work for objects with
variant equals.
If you Google around for the proper implementation of equals(), you will
find many serious sites that discuss it at length, yet provide examples
like:
public boolean equals(Object aThat){
if ( this == aThat ) return true;
if ( !(aThat instanceof Car) ) return false;
Car that = (Car)aThat;
return
EqualsUtil.areEqual(this.fName, that.fName)&&
EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&&
EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&&
EqualsUtil.areEqual(this.fColor, that.fColor)&&
Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
//array!
}
Ironically, that is explicitly against Wolfgang's suggestion above!
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
8:39 a.m.
Actually this can be done without two collections.
Just use a HashMap and mainta a List of the "clashed" identities. To
operate on the actualy identity, such as remove. Lookup the List, via
normal map.get( key ) then iterate the list to remove the identity. If
the list is empty, remove the key. reverse removes the entity, makes it
change, and then adds it back in. Adding is the process of looking for
an existing key and adding to it's clash list, or creating a new key
with an entry of one.
This would provide identity safe update/removal/reverse will providing a
set view of the world. I suspect overhead would be < 15% if done right.
It should be done as a custom accumulate function for now, and we should
profile the performance characteristics of the two - as there are some
optimizations that can be made, such as only lazy storying the value as
a list only when there is more than one entry. If you are willing to
code this up, we can give you pointers on irc:
http://www.jboss.org/drools/irc
http://www.athico.com/Getting_Involved/gettinginvolved.html
Mark
On 28/02/2012 07:25, Mark Proctor wrote:
The getResults()
would probably have to change from...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( data.map.keySet() );
}
...to...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( new
HashSet(data.map.keySet())
);
}
The proposed getResult() results in a full HashSet copy for each
change of Rete, that performance hit would be unbearable for most people.
I do agree that there is some merit to what you are asking, but we
can't give that perf hit to everyone. It would be better to maintain
two collections, one identity and one set based. And some how only
change in set what was relevant to the change in identity, you might
need to build a custom hash table for this - see AbstractHashTable in
drools which can easily be extended for custom keys and custom
add/remove methods.
Mark
On 27/02/2012 16:53, SirMungus wrote:
> Mark Proctor wrote
>> accumulate supports custom functions, just add your own version of
>> collectSet that uses IdentityMap.
>>
> I could do that, but please let me take one more stab, just in case I
> haven't been clear enough.
>
> 1) I am not suggesting that collectSet() return an identity-based
> set. I am
> content to have it return a normal set, though I had early confusion
> on that
> point.
>
> 2) The discussion above about stated vs. justified facts turns out to
> be a
> distraction to the main point of the bug that I believe to be in
> CollectSetAccumulateFunction. My failure to understand the
> distinction is
> not germane to the bug.
>
> 3) I am not disagreeing with the value behind implementing equals()
> to only
> take into account invariant fields, though I am stating that goes
> beyond the
> explicity documentation of equals()/hashCode() and is explicitly *not*
> followed by many classes in the JRE (most notably, collections).
>
> There is nothing about the purpose of collectSet() that logically
> suggests
> that it should only work with objects that have invariant
> equals()/hashCode() functions. If five objects match a rule, they
> should be
> able to be collected into a set (which may end up with fewer than five
> objects). If things change such that only three facts match the rule,
> they
> should still be able to be collected into a set. There's no compelling
> reason why that should only apply to objects with invariant equals().
> And it
> seems to me that you rarely want to leave an NPE in code, rather than
> catching it and throwing a more intelligible exception (e.g., "Object
> to be
> removed from collectSet() was not found").
>
> If the internal maintenance of state in CollectSetAccumulateFunction
> used
> identity semantics and applied the equality semantics only when you
> asked it
> for its results, it would maintain the same external behavior
> (returning a
> "normal" equality-based set) without failing to work for objects with
> variant equals.
>
> If you Google around for the proper implementation of equals(), you will
> find many serious sites that discuss it at length, yet provide examples
> like:
>
> public boolean equals(Object aThat){
> if ( this == aThat ) return true;
> if ( !(aThat instanceof Car) ) return false;
> Car that = (Car)aThat;
> return
> EqualsUtil.areEqual(this.fName, that.fName)&&
> EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&&
> EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&&
> EqualsUtil.areEqual(this.fColor, that.fColor)&&
> Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
> //array!
> }
>
> Ironically, that is explicitly against Wolfgang's suggestion above!
>
>
> --
> View this message in context:
>
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
> Sent from the Drools: Developer (committer) mailing list mailing list
> archive at Nabble.com.
> _______________________________________________
> rules-dev mailing list
> rules-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
11:28 a.m.
How do you find the List for removal by identity when the key has
already been changed?
-W
On 28/02/2012, Mark Proctor <mproctor(a)codehaus.org> wrote:
Actually this can be done without two collections.
Just use a HashMap and mainta a List of the "clashed" identities. To
operate on the actualy identity, such as remove. Lookup the List, via
normal map.get( key ) then iterate the list to remove the identity. If
the list is empty, remove the key. reverse removes the entity, makes it
change, and then adds it back in. Adding is the process of looking for
an existing key and adding to it's clash list, or creating a new key
with an entry of one.
This would provide identity safe update/removal/reverse will providing a
set view of the world. I suspect overhead would be < 15% if done right.
It should be done as a custom accumulate function for now, and we should
profile the performance characteristics of the two - as there are some
optimizations that can be made, such as only lazy storying the value as
a list only when there is more than one entry. If you are willing to
code this up, we can give you pointers on irc:
http://www.jboss.org/drools/irc
http://www.athico.com/Getting_Involved/gettinginvolved.html
Mark
On 28/02/2012 07:25, Mark Proctor wrote:
> The getResults()
> would probably have to change from...
>
> public Object getResult(Serializable context) throws Exception {
> CollectListData data = (CollectListData) context;
> return Collections.unmodifiableSet( data.map.keySet() );
> }
>
> ...to...
>
> public Object getResult(Serializable context) throws Exception {
> CollectListData data = (CollectListData) context;
> return Collections.unmodifiableSet( new
> HashSet(data.map.keySet())
> );
> }
>
> The proposed getResult() results in a full HashSet copy for each
> change of Rete, that performance hit would be unbearable for most people.
>
> I do agree that there is some merit to what you are asking, but we
> can't give that perf hit to everyone. It would be better to maintain
> two collections, one identity and one set based. And some how only
> change in set what was relevant to the change in identity, you might
> need to build a custom hash table for this - see AbstractHashTable in
> drools which can easily be extended for custom keys and custom
> add/remove methods.
>
> Mark
>
>
> On 27/02/2012 16:53, SirMungus wrote:
>> Mark Proctor wrote
>>> accumulate supports custom functions, just add your own version of
>>> collectSet that uses IdentityMap.
>>>
>> I could do that, but please let me take one more stab, just in case I
>> haven't been clear enough.
>>
>> 1) I am not suggesting that collectSet() return an identity-based
>> set. I am
>> content to have it return a normal set, though I had early confusion
>> on that
>> point.
>>
>> 2) The discussion above about stated vs. justified facts turns out to
>> be a
>> distraction to the main point of the bug that I believe to be in
>> CollectSetAccumulateFunction. My failure to understand the
>> distinction is
>> not germane to the bug.
>>
>> 3) I am not disagreeing with the value behind implementing equals()
>> to only
>> take into account invariant fields, though I am stating that goes
>> beyond the
>> explicity documentation of equals()/hashCode() and is explicitly *not*
>> followed by many classes in the JRE (most notably, collections).
>>
>> There is nothing about the purpose of collectSet() that logically
>> suggests
>> that it should only work with objects that have invariant
>> equals()/hashCode() functions. If five objects match a rule, they
>> should be
>> able to be collected into a set (which may end up with fewer than five
>> objects). If things change such that only three facts match the rule,
>> they
>> should still be able to be collected into a set. There's no compelling
>> reason why that should only apply to objects with invariant equals().
>> And it
>> seems to me that you rarely want to leave an NPE in code, rather than
>> catching it and throwing a more intelligible exception (e.g., "Object
>> to be
>> removed from collectSet() was not found").
>>
>> If the internal maintenance of state in CollectSetAccumulateFunction
>> used
>> identity semantics and applied the equality semantics only when you
>> asked it
>> for its results, it would maintain the same external behavior
>> (returning a
>> "normal" equality-based set) without failing to work for objects with
>> variant equals.
>>
>> If you Google around for the proper implementation of equals(), you will
>> find many serious sites that discuss it at length, yet provide examples
>> like:
>>
>> public boolean equals(Object aThat){
>> if ( this == aThat ) return true;
>> if ( !(aThat instanceof Car) ) return false;
>> Car that = (Car)aThat;
>> return
>> EqualsUtil.areEqual(this.fName, that.fName)&&
>> EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&&
>> EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&&
>> EqualsUtil.areEqual(this.fColor, that.fColor)&&
>> Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
>> //array!
>> }
>>
>> Ironically, that is explicitly against Wolfgang's suggestion above!
>>
>>
>> --
>> View this message in context:
>>
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
>> Sent from the Drools: Developer (committer) mailing list mailing list
>> archive at Nabble.com.
>> _______________________________________________
>> rules-dev mailing list
>> rules-dev(a)lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/rules-dev
>
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
4:22 p.m.
Hi all,
Sorry for being late to reply to this thread. A few comments:
a) Please note that clashes objects was taken into account in the design of
the solution, but there is a bug in there.
b) If you look at [1] you will see that the value that is being collected
is already cached by the engine by fact handle ID and used on reverse. So
that takes care of expressions, for instance, where a new object is created
every time. For instance:
collectSet( $firstName + $lastName )
In that case, every time the expression is evaluated, it will create a new
immutable String object that we tract by fact ID, cache it, and properly
manage with equality semantics.
c) The bug seems to happen when the expression in the function returns a
mutable object, as observed by SirMungus. E.g.:
collectSet( $person )
In this case, even if the original object is cached and re-used in [1],
because it changed hashCodes(), it is not found in [2], as reported.
d) The problem is that we are collecting *sets* of objects, and because of
that, we have to use equality semantics to check for duplicates. I was
using a java Map to track that, and that works find for the use case (b)
above, but obviously breaks on use case (c). Moving to identity does not
seem correct to me as it would work for use case (c), but no longer work
for (b).
I don't have a solution yet, as I am involved in another large complex
task, but I will re-evaluate this as soon as possible. Meanwhile, if
someone has a suggestion that supports both cases above, please let us know.
Thank you,
Edson
[1]
https://github.com/droolsjbpm/drools/blob/master/drools-core/src/main/jav...
[2]
https://github.com/droolsjbpm/drools/blob/master/drools-core/src/main/jav...
On Tue, Feb 28, 2012 at 5:28 AM, Wolfgang Laun <wolfgang.laun(a)gmail.com>wrote:
How do you find the List for removal by identity when the key has
already been changed?
-W
On 28/02/2012, Mark Proctor <mproctor(a)codehaus.org> wrote:
> Actually this can be done without two collections.
>
> Just use a HashMap and mainta a List of the "clashed" identities. To
> operate on the actualy identity, such as remove. Lookup the List, via
> normal map.get( key ) then iterate the list to remove the identity. If
> the list is empty, remove the key. reverse removes the entity, makes it
> change, and then adds it back in. Adding is the process of looking for
> an existing key and adding to it's clash list, or creating a new key
> with an entry of one.
>
> This would provide identity safe update/removal/reverse will providing a
> set view of the world. I suspect overhead would be < 15% if done right.
>
> It should be done as a custom accumulate function for now, and we should
> profile the performance characteristics of the two - as there are some
> optimizations that can be made, such as only lazy storying the value as
> a list only when there is more than one entry. If you are willing to
> code this up, we can give you pointers on irc:
> http://www.jboss.org/drools/irc
> http://www.athico.com/Getting_Involved/gettinginvolved.html
>
> Mark
> On 28/02/2012 07:25, Mark Proctor wrote:
>> The getResults()
>> would probably have to change from...
>>
>> public Object getResult(Serializable context) throws Exception {
>> CollectListData data = (CollectListData) context;
>> return Collections.unmodifiableSet( data.map.keySet() );
>> }
>>
>> ...to...
>>
>> public Object getResult(Serializable context) throws Exception {
>> CollectListData data = (CollectListData) context;
>> return Collections.unmodifiableSet( new
>> HashSet(data.map.keySet())
>> );
>> }
>>
>> The proposed getResult() results in a full HashSet copy for each
>> change of Rete, that performance hit would be unbearable for most
people.
>>
>> I do agree that there is some merit to what you are asking, but we
>> can't give that perf hit to everyone. It would be better to maintain
>> two collections, one identity and one set based. And some how only
>> change in set what was relevant to the change in identity, you might
>> need to build a custom hash table for this - see AbstractHashTable in
>> drools which can easily be extended for custom keys and custom
>> add/remove methods.
>>
>> Mark
>>
>>
>> On 27/02/2012 16:53, SirMungus wrote:
>>> Mark Proctor wrote
>>>> accumulate supports custom functions, just add your own version of
>>>> collectSet that uses IdentityMap.
>>>>
>>> I could do that, but please let me take one more stab, just in case I
>>> haven't been clear enough.
>>>
>>> 1) I am not suggesting that collectSet() return an identity-based
>>> set. I am
>>> content to have it return a normal set, though I had early confusion
>>> on that
>>> point.
>>>
>>> 2) The discussion above about stated vs. justified facts turns out to
>>> be a
>>> distraction to the main point of the bug that I believe to be in
>>> CollectSetAccumulateFunction. My failure to understand the
>>> distinction is
>>> not germane to the bug.
>>>
>>> 3) I am not disagreeing with the value behind implementing equals()
>>> to only
>>> take into account invariant fields, though I am stating that goes
>>> beyond the
>>> explicity documentation of equals()/hashCode() and is explicitly *not*
>>> followed by many classes in the JRE (most notably, collections).
>>>
>>> There is nothing about the purpose of collectSet() that logically
>>> suggests
>>> that it should only work with objects that have invariant
>>> equals()/hashCode() functions. If five objects match a rule, they
>>> should be
>>> able to be collected into a set (which may end up with fewer than five
>>> objects). If things change such that only three facts match the rule,
>>> they
>>> should still be able to be collected into a set. There's no compelling
>>> reason why that should only apply to objects with invariant equals().
>>> And it
>>> seems to me that you rarely want to leave an NPE in code, rather than
>>> catching it and throwing a more intelligible exception (e.g., "Object
>>> to be
>>> removed from collectSet() was not found").
>>>
>>> If the internal maintenance of state in CollectSetAccumulateFunction
>>> used
>>> identity semantics and applied the equality semantics only when you
>>> asked it
>>> for its results, it would maintain the same external behavior
>>> (returning a
>>> "normal" equality-based set) without failing to work for objects
with
>>> variant equals.
>>>
>>> If you Google around for the proper implementation of equals(), you
will
>>> find many serious sites that discuss it at length, yet provide examples
>>> like:
>>>
>>> public boolean equals(Object aThat){
>>> if ( this == aThat ) return true;
>>> if ( !(aThat instanceof Car) ) return false;
>>> Car that = (Car)aThat;
>>> return
>>> EqualsUtil.areEqual(this.fName, that.fName)&&
>>> EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors)&&
>>> EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage)&&
>>> EqualsUtil.areEqual(this.fColor, that.fColor)&&
>>> Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
>>> //array!
>>> }
>>>
>>> Ironically, that is explicitly against Wolfgang's suggestion above!
>>>
>>>
>>> --
>>> View this message in context:
>>>
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
>>> Sent from the Drools: Developer (committer) mailing list mailing list
>>> archive at Nabble.com.
>>> _______________________________________________
>>> rules-dev mailing list
>>> rules-dev(a)lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/rules-dev
>>
>
> _______________________________________________
> rules-dev mailing list
> rules-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
>
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
--
Edson Tirelli
JBoss Drools Core Development
JBoss by Red Hat @ www.jboss.com
5:42 p.m.
Mark Proctor wrote
The getResults()
would probably have to change from...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( data.map.keySet() );
}
...to...
public Object getResult(Serializable context) throws Exception {
CollectListData data = (CollectListData) context;
return Collections.unmodifiableSet( new
HashSet(data.map.keySet())
);
}
The proposed getResult() results in a full HashSet copy for each change of
Rete, that performance hit would be unbearable for most people.
Well, I did not the concern in my original post, but I think it's a concern
that really should be tested. If it was "unbearable", then presumably no one
would ever use the current CollectListAccumulateFunction implementation,
because it does an O(N) operation for each reverse() on the list, not just
in the getResults() call at the end, which is presumably called no more
often.
I suspect that any performance degradation in your tests would be within the
error bar of the tests. I put together a quick JUnit:
@Test
public void testHashSet() {
Map<Integer, Integer> map = new IdentityHashMap<Integer,Integer>();
for (int i = 0; i < 100; i++) {
map.put(i, i);
}
long startMillis = System.currentTimeMillis();
for (int i = 0; i < 10000; i++) {
Set<Integer> set = new HashSet<Integer>(map.keySet());
}
long endMillis = System.currentTimeMillis();
System.out.println(endMillis - startMillis);
}
On my machine, running 10,000 new HashSet() operations on the key set of an
IdentityHashMap with 100 keys takes about 100 milliseconds. I don't have
much familiarity with the Drools test cases, the average customer's rule
base, nor the prevalence in those rule bases of using collectSet(). But,
that sounds like a small performance concern to me.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
5:55 p.m.
Edson Tirelli-4 wrote
a) Please note that clashes objects was taken into account in the design
of the solution, but there is a bug in there.
I eventually surmised that from seeing the use of MutableInt within it.
Edson Tirelli-4 wrote
b) If you look at [1] you will see that the value that is being collected
is already cached by the engine by fact handle ID and used on reverse. So
that takes care of expressions, for instance, where a new object is
created every time. For instance:
collectSet( $firstName + $lastName )
In that case, every time the expression is evaluated, it will create a new
immutable String object that we tract by fact ID, cache it, and properly
manage with equality semantics.
* * *
d) The problem is that we are collecting *sets* of objects, and because of
that, we have to use equality semantics to check for duplicates. I was
using a java Map to track that, and that works find for the use case (b)
above, but obviously breaks on use case (c). Moving to identity does not
seem correct to me as it would work for use case (c), but no longer work
for (b).
I don't have a solution yet, as I am involved in another large complex
task, but I will re-evaluate this as soon as possible. Meanwhile, if
someone has a suggestion that supports both cases above, please let us
know.
Your scenario (b) is a very interesting point. I'm getting a bit beyond my
depth here, but when the existing node in the RETE tree changes such that it
realizes it will no longer contribute to the rule, does it re-evaluate the
"$firstname + $lastname" and then try to remove it, or does it use a cached
FactHandle from its prior evaluation. If it re-evaluates, I agree that it
is a problem. If it uses the FactHandle from before, then the
IdentityHashMap implementation should be fine, because the equals()
semantics will be applied in the getResults(). Your text above reads to me
as though the latter is the case. So, aren't we fine here?
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
6:43 p.m.
I think the key here is that the functions are plugable and the contract
between the engine and the function [1] is a single object that is passed
into the function methods:
public void accumulate(Serializable context,
Object value);
*
*
public void reverse(Serializable context,
Object value) throws Exception;
So, while the engine knows how to execute and obtain the value of the
expression, e.g.:
a) collectSet( $firstName + $lastName )
b) collectSet( $person )
It only sends to the function the actual result. In case of (a), an
immutable String object, in case of (b), a mutable Person object. The
engine does use a cached value on the retract, so the same instances of
both String and Person are used on the reverse method call, but the
function itself doesn't know if that is a mutable or immutable object. It
also doesn't know if it is an object resulting from a complex expression
(a) or a fact straight from the working memory (b).
As you noticed already, the function uses the resulting object as a key
in the map because the goal is indeed to match equals() objects and filter
them to return a set, no duplicates. Of course, because in case of (b) the
hashcode changes, java no longer finds the key to remove it from the map.
The problem of using a straight identity map is that it will no longer
collect sets of equals() object, but sets of identical objects, what is
obviously not the intent of the function. For instance, in case (a), if two
objects have the same first and last name, there will be 2 instances of the
resulting object created, even if they are equals(). That will break the
function.
Using a combination of lists and maps to maintain the values might work,
but as you mentioned it will give us O(N) performance. Anyway, I need time
to think about it, and unfortunately extremely busy at the moment with
another task.
Thanks for looking into it and if you find a solution, please let us
know. :)
Edson
[1]
https://github.com/droolsjbpm/droolsjbpm-knowledge/blob/master/knowledge-...
On Tue, Feb 28, 2012 at 11:55 AM, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
Edson Tirelli-4 wrote
>
> a) Please note that clashes objects was taken into account in the design
> of the solution, but there is a bug in there.
>
I eventually surmised that from seeing the use of MutableInt within it.
Edson Tirelli-4 wrote
>
> b) If you look at [1] you will see that the value that is being collected
> is already cached by the engine by fact handle ID and used on reverse. So
> that takes care of expressions, for instance, where a new object is
> created every time. For instance:
>
> collectSet( $firstName + $lastName )
>
> In that case, every time the expression is evaluated, it will create a
new
> immutable String object that we tract by fact ID, cache it, and properly
> manage with equality semantics.
> * * *
> d) The problem is that we are collecting *sets* of objects, and because
of
> that, we have to use equality semantics to check for duplicates. I was
> using a java Map to track that, and that works find for the use case (b)
> above, but obviously breaks on use case (c). Moving to identity does not
> seem correct to me as it would work for use case (c), but no longer work
> for (b).
>
> I don't have a solution yet, as I am involved in another large complex
> task, but I will re-evaluate this as soon as possible. Meanwhile, if
> someone has a suggestion that supports both cases above, please let us
> know.
>
Your scenario (b) is a very interesting point. I'm getting a bit beyond my
depth here, but when the existing node in the RETE tree changes such that
it
realizes it will no longer contribute to the rule, does it re-evaluate the
"$firstname + $lastname" and then try to remove it, or does it use a cached
FactHandle from its prior evaluation. If it re-evaluates, I agree that it
is a problem. If it uses the FactHandle from before, then the
IdentityHashMap implementation should be fine, because the equals()
semantics will be applied in the getResults(). Your text above reads to me
as though the latter is the case. So, aren't we fine here?
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
--
Edson Tirelli
JBoss Drools Core Development
JBoss by Red Hat @ www.jboss.com
7:01 p.m.
Edson Tirelli-4 wrote
As you noticed already, the function uses the resulting object as a key
in the map because the goal is indeed to match equals() objects and filter
them to return a set, no duplicates. Of course, because in case of (b) the
hashcode changes, java no longer finds the key to remove it from the map.
The problem of using a straight identity map is that it will no longer
collect sets of equals() object, but sets of identical objects, what is
obviously not the intent of the function. For instance, in case (a), if
two
objects have the same first and last name, there will be 2 instances of
the
resulting object created, even if they are equals(). That will break the
function.
Thanks for the attention you've been able to give this, Edson.
The solution I've proposed doesn't break the equals() contact of the
resulting set; it just applies it at getResult() time, rather than during
accumulate()/reverse(). So, the main question would be whether the creation
of a new HashSet in getResult() is too costly, which I've discussed
elsewhere on this thread.
Are there existing stress tests that could test that empirically?
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
7:07 p.m.
I see what you are saying and I think it works. Although, it will be
heavier, it would cover scenarios not covered by the current implementation.
My suggestion is if you can, please create a test case for the problem,
change the collectSet accumulate function to do what you propose and send a
pull request. I will review it and apply. I think in this case, making
user's life easier by not requiring immutability might trump performance,
but we need to evaluate some tests just in case.
Edson
On Tue, Feb 28, 2012 at 1:01 PM, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
Edson Tirelli-4 wrote
>
> As you noticed already, the function uses the resulting object as a
key
> in the map because the goal is indeed to match equals() objects and
filter
> them to return a set, no duplicates. Of course, because in case of (b)
the
> hashcode changes, java no longer finds the key to remove it from the map.
>
> The problem of using a straight identity map is that it will no longer
> collect sets of equals() object, but sets of identical objects, what is
> obviously not the intent of the function. For instance, in case (a), if
> two
> objects have the same first and last name, there will be 2 instances of
> the
> resulting object created, even if they are equals(). That will break the
> function.
>
Thanks for the attention you've been able to give this, Edson.
The solution I've proposed doesn't break the equals() contact of the
resulting set; it just applies it at getResult() time, rather than during
accumulate()/reverse(). So, the main question would be whether the creation
of a new HashSet in getResult() is too costly, which I've discussed
elsewhere on this thread.
Are there existing stress tests that could test that empirically?
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
--
Edson Tirelli
JBoss Drools Core Development
JBoss by Red Hat @ www.jboss.com
7:45 p.m.
Edson Tirelli-4 wrote
I see what you are saying and I think it works. Although, it will be
heavier, it would cover scenarios not covered by the current
implementation.
My suggestion is if you can, please create a test case for the problem,
change the collectSet accumulate function to do what you propose and send
a
pull request. I will review it and apply. I think in this case, making
user's life easier by not requiring immutability might trump performance,
but we need to evaluate some tests just in case.
Edson, thanks for the offer. I'll see what I can do. Since I work at a
financial services company, I practically have to get Obama's permission to
do anything official on an open source project. :)
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
7:55 p.m.
I am sure he would not deny that... :) you are helping to make the world
a better place... :)
In any case, please open a JIRA so that this does not get lost in the
e-mails. It is a low hanging fruit, very isolated and simple to fix. So
anyone looking for a starting work to contribute to the project can do it.
Otherwise I will do it when I get the time.
Thanks,
Edson
On Tue, Feb 28, 2012 at 1:45 PM, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
Edson Tirelli-4 wrote
>
> I see what you are saying and I think it works. Although, it will be
> heavier, it would cover scenarios not covered by the current
> implementation.
>
> My suggestion is if you can, please create a test case for the
problem,
> change the collectSet accumulate function to do what you propose and send
> a
> pull request. I will review it and apply. I think in this case, making
> user's life easier by not requiring immutability might trump performance,
> but we need to evaluate some tests just in case.
>
Edson, thanks for the offer. I'll see what I can do. Since I work at a
financial services company, I practically have to get Obama's permission to
do anything official on an open source project. :)
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
--
Edson Tirelli
JBoss Drools Core Development
JBoss by Red Hat @ www.jboss.com
8:39 p.m.
I have cloned it and running benchmarks on it. Might not be able to
complete it tonight.
-W
On 28 February 2012 19:55, Edson Tirelli <ed.tirelli(a)gmail.com> wrote:
I am sure he would not deny that... :) you are helping to make the
world a better place... :)
In any case, please open a JIRA so that this does not get lost in the
e-mails. It is a low hanging fruit, very isolated and simple to fix. So
anyone looking for a starting work to contribute to the project can do it.
Otherwise I will do it when I get the time.
Thanks,
Edson
On Tue, Feb 28, 2012 at 1:45 PM, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
>
> Edson Tirelli-4 wrote
> >
> > I see what you are saying and I think it works. Although, it will be
> > heavier, it would cover scenarios not covered by the current
> > implementation.
> >
> > My suggestion is if you can, please create a test case for the
> problem,
> > change the collectSet accumulate function to do what you propose and
> send
> > a
> > pull request. I will review it and apply. I think in this case, making
> > user's life easier by not requiring immutability might trump
> performance,
> > but we need to evaluate some tests just in case.
> >
> Edson, thanks for the offer. I'll see what I can do. Since I work at a
> financial services company, I practically have to get Obama's permission
> to
> do anything official on an open source project. :)
>
> --
> View this message in context:
>
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
> Sent from the Drools: Developer (committer) mailing list mailing list
> archive at Nabble.com.
> _______________________________________________
> rules-dev mailing list
> rules-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
>
--
Edson Tirelli
JBoss Drools Core Development
JBoss by Red Hat @ www.jboss.com
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
9:41 a.m.
Here are some first results from comparing collectSet (as in 5.3.0) to
a similar accumulate function using an IdentitiyHashMap<Object,Void>
in its context and returns a true Set as a of the key set as its
result. The reported times are the elapsed times of firing a single
rule doing an accumulate into a Set from all String objects in WM.
(1) An extreme case: insert many different String facts and collect
them into a single set. Strings were composed by concatenating "s" and
i = 0(1)N-1
// different Strings
// collectSet
// N= 10000 2.542s
// N= 20000 11.118s
// N= 100000 198.839s
// ----------------
// collectIdentity
// N= 10000 3.944s
// N= 20000 18.514s
// N= 100000 832.093s
// ===============
(2) Another extreme case: insert many equal String facts as distinct
objects and collect them into a single set. String value "s12345".
// equal Strings
// collectSet
// N= 10000 0.419s
// N= 20000 0.558s
// N= 100000 0.816s
// ---------------
// collectIdentity
// N= 10000 2.940s
// N= 20000 13.048s
// N= 100000 522.286s
(It's faster due to not having to construct the string.)
It's obvious that the continuing growth of a HashMap with its calls to
resize for increasing capacities of 16, 32, 64,... and the rehashing
of all entries is costly.
If your final Set is just a small portion of a (large number) of
collected elements, maintaining the IdentityHashMap is costly whereas
the true set of objects remains at minimum size throughout.
-W
On 28 February 2012 20:39, Wolfgang Laun <wolfgang.laun(a)gmail.com> wrote:
I have cloned it and running benchmarks on it. Might not be able to complete it tonight.
-W
On 28 February 2012 19:55, Edson Tirelli <ed.tirelli(a)gmail.com> wrote:
>
>
> I am sure he would not deny that... :) you are helping to make the world a better
place... :)
>
> In any case, please open a JIRA so that this does not get lost in the e-mails. It
is a low hanging fruit, very isolated and simple to fix. So anyone looking for a starting
work to contribute to the project can do it. Otherwise I will do it when I get the time.
>
> Thanks,
> Edson
>
> On Tue, Feb 28, 2012 at 1:45 PM, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
>>
>>
>> Edson Tirelli-4 wrote
>> >
>> > I see what you are saying and I think it works. Although, it will be
>> > heavier, it would cover scenarios not covered by the current
>> > implementation.
>> >
>> > My suggestion is if you can, please create a test case for the problem,
>> > change the collectSet accumulate function to do what you propose and send
>> > a
>> > pull request. I will review it and apply. I think in this case, making
>> > user's life easier by not requiring immutability might trump
performance,
>> > but we need to evaluate some tests just in case.
>> >
>> Edson, thanks for the offer. I'll see what I can do. Since I work at a
>> financial services company, I practically have to get Obama's permission to
>> do anything official on an open source project. :)
>>
>> --
>> View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
>> Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
>> _______________________________________________
>> rules-dev mailing list
>> rules-dev(a)lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/rules-dev
>
>
>
>
> --
> Edson Tirelli
> JBoss Drools Core Development
> JBoss by Red Hat @ www.jboss.com
>
> _______________________________________________
> rules-dev mailing list
> rules-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev
>
11:59 p.m.
laune wrote
Here are some first results from comparing collectSet (as in 5.3.0) to
a similar accumulate function using an IdentitiyHashMap<Object,Void>
in its context and returns a true Set as a of the key set as its
result. The reported times are the elapsed times of firing a single
rule doing an accumulate into a Set from all String objects in WM.
Out of curiosity, I did a quick timing test of repeated accumulate() calls
(without getResults() calls) using IdentityHashMap vs. HashMap using your
"s##" strings. While there should have been no particular reason for the
performance to differ between the two (indeed, one would think identity
would be faster because the identity hash function is natively implemented
and presumably much faster), I consistently found the IdentityHashMap to
take twice the time for its put() calls. Looking at the source for the two
classes, I was surprised to see that they have completely separate
implementations.
That's pretty astonishing. I would have figured that HashMap would have
protected methods like computeHash(key) and testEquals(key1,key2) and that
those would be overridden in an IdentityHashMap to use
System.identityHashCode() and ==, rather than hashCode() and equals().
My best guess is that someone has spent more time optimizing HashMap than
IdentityHashMap.
So, there would undoubtedly be a performance impact, even not taking into
account the "new HashSet()" in the getResults() call. Whether it would be
significant enough in real world scenarios to warrant a much more complex
implementation requires more knowledge of Drools's user base than I possess,
for sure. My guess is that Wolfgang's second scenario above is generally
unrealistic. But, if it is routine for rules with collectSet() to end up
with sets involving thousands of objects like the second scenario, that
could certainly be an issue.
I will try to get the requested JIRA in soon, but I won't be able to give
much attention to this until maybe a week from now. I've fallen a bit
behind because of all this. :)
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
12:11 a.m.
SirMungus wrote
So, there would undoubtedly be a performance impact, even not taking into
account the "new HashSet()" in the getResults() call. Whether it would be
significant enough in real world scenarios to warrant a much more complex
implementation requires more knowledge of Drools's user base than I
possess, for sure. My guess is that Wolfgang's second scenario above is
generally unrealistic. But, if it is routine for rules with collectSet()
to end up with sets involving thousands of objects like the /second/
scenario, that could certainly be an issue.
Goodness. I meant to say that the first scenario (instead of "second"
italicized above) could potentially be realistic. The larger the sets,
larger the penalty each time getResults() is called.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
7:26 a.m.
IdentityHashMap is kinda interesting, I thought the same when I first
looked at it.
It doesnt' create Entry buckets to store hash clashes. It instead just
moves to the next free space for two elements in the array and adds the
adds the key, followed by the value in the next array element . So in
theory it's GC free, but if the map is too densely populated it'll have
a longer search for free space.
Mark
On 29/02/2012 22:59, SirMungus wrote:
laune wrote
> Here are some first results from comparing collectSet (as in 5.3.0) to
> a similar accumulate function using an IdentitiyHashMap<Object,Void>
> in its context and returns a true Set as a of the key set as its
> result. The reported times are the elapsed times of firing a single
> rule doing an accumulate into a Set from all String objects in WM.
>
Out of curiosity, I did a quick timing test of repeated accumulate() calls
(without getResults() calls) using IdentityHashMap vs. HashMap using your
"s##" strings. While there should have been no particular reason for the
performance to differ between the two (indeed, one would think identity
would be faster because the identity hash function is natively implemented
and presumably much faster), I consistently found the IdentityHashMap to
take twice the time for its put() calls. Looking at the source for the two
classes, I was surprised to see that they have completely separate
implementations.
That's pretty astonishing. I would have figured that HashMap would have
protected methods like computeHash(key) and testEquals(key1,key2) and that
those would be overridden in an IdentityHashMap to use
System.identityHashCode() and ==, rather than hashCode() and equals().
My best guess is that someone has spent more time optimizing HashMap than
IdentityHashMap.
So, there would undoubtedly be a performance impact, even not taking into
account the "new HashSet()" in the getResults() call. Whether it would be
significant enough in real world scenarios to warrant a much more complex
implementation requires more knowledge of Drools's user base than I possess,
for sure. My guess is that Wolfgang's second scenario above is generally
unrealistic. But, if it is routine for rules with collectSet() to end up
with sets involving thousands of objects like the second scenario, that
could certainly be an issue.
I will try to get the requested JIRA in soon, but I won't be able to give
much attention to this until maybe a week from now. I've fallen a bit
behind because of all this. :)
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list archive at
Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
7:39 a.m.
Given time, I can concoct a test case where the percentage of equals
varies between 0 and 100%. But whatever these results might be, I
think that the findings up to now indicate that collectSet should be left
as it is, and any desire for a different behaviour should be satisfied
by a separate accumulate function, say, collectBag, returning the
key set of an IdentityHashMap. (Clarifying the reasons of the implementation
differences might be interesting.)
-W
On 1 March 2012 00:11, SirMungus <Patrick_Rusk(a)ssga.com> wrote:
SirMungus wrote
>
> So, there would undoubtedly be a performance impact, even not taking into
> account the "new HashSet()" in the getResults() call. Whether it would be
> significant enough in real world scenarios to warrant a much more complex
> implementation requires more knowledge of Drools's user base than I
> possess, for sure. My guess is that Wolfgang's second scenario above is
> generally unrealistic. But, if it is routine for rules with collectSet()
> to end up with sets involving thousands of objects like the /second/
> scenario, that could certainly be an issue.
>
Goodness. I meant to say that the first scenario (instead of "second"
italicized above) could potentially be realistic. The larger the sets,
larger the penalty each time getResults() is called.
--
View this message in context:
http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFun...
Sent from the Drools: Developer (committer) mailing list mailing list
archive at Nabble.com.
_______________________________________________
rules-dev mailing list
rules-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/rules-dev
4438
days inactive
4444
days old
26 comments
4 participants
participants (4)
-
Edson Tirelli -
Mark Proctor -
SirMungus -
Wolfgang Laun