performance problem caused by Sets.newSetFromMap() #1069
Comments
|
Original comment posted by wasserman.louis on 2012-07-16 at 07:38 PM Hmmmm. There are arguments to be made for both sides, but I'm not honestly sure it matters one way or the other. Either we can have something completely equivalent to Collections.newSetFromMap in JDK 6, or...we can worry about the performance of a highly rare edge case. Eh. Labels: |
|
Original comment posted by wasserman.louis on 2012-07-16 at 07:38 PM Hmmmm. There are arguments to be made for both sides, but I'm not honestly sure it matters one way or the other. Either we can have something completely equivalent to Collections.newSetFromMap in JDK 6, or...we can worry about the performance of a highly rare edge case. Eh. |
|
Original comment posted by adi175652q on 2012-07-16 at 08:00 PM Hi Louis, We are just replacing one method call with another, which does not Best, Adrian |
|
Original comment posted by kevinb@google.com on 2012-07-16 at 09:39 PM Are you using JDK 1.6, and if so, why not use java.util.Collections.newSetFromMap? |
|
Original comment posted by adi175652q on 2012-07-16 at 09:57 PM Hi Louis, Yes, I am using JDK 1.6, but it has the same problem. I do not expect Best, Adrian |
|
Original comment posted by adi175652q on 2012-07-18 at 12:47 AM Hi Louis,
I missed this part from Comment 1. But they are completely return s.removeAll(c); and return removeAllImpl(s, c); do exactly the same thing. You are not changing the behavior in any Best, Adrian |
|
Original comment posted by wasserman.louis on 2012-07-19 at 10:32 AM This issue still appears incompatible with Kevin's plan in issue 1001. I think I'll push the change for the moment, and it'll probably live on in the JDK 5 backport, but I think I still support the idea of deprecating this functionality and pointing Java 6 users to the JDK. |
|
Original comment posted by kurt.kluever on 2012-07-24 at 05:32 PM In Guava for 1.6, we'll just deprecate and delegate to the JDK version. In the backport, we'll probably stick with the existing impl. Status: |
gissuebot
added
migrated
package=collect
status=triaged
type=performance
|
We're about to deprecate this method. |
Original issue created by adi175652q on 2012-07-16 at 06:59 PM
Hi,
I am encountering a performance problem caused by Sets.newSetFromMap().
It appears in the latest Guava version (16 July). I attached a test
that exposes this problem and a one-line patch that fixes it. On my
machine, for this test, the patch provides a 386X speedup.
To run the test, just do:
$ java Test
The output for the un-patched version is:
Time is: 5027
The output for the patched version is:
Time is 13
This problem is similar to the previously fixed Issue 1013. The
problem is that:
Sets.newSetFromMap()
returns a SetFromMap (private class), which defines its
removeAll(Collection<?> c) as:
@Override public boolean removeAll(Collection<?> c) {
return s.removeAll(c);
}
As per the fix of Issue 1013, this should be:
@Override public boolean removeAll(Collection<?> c) {
return removeAllImpl(s, c);
}
The fix for Issue 1013 replaces several calls to "removeAll" with
calls to "removeAllImpl"
(e.g., in ForwardingSet.standardRemoveAll(Collection<?> collection)),
but not here.
Is this a bug, or am I misunderstanding the intended behavior? If so,
can you please confirm that the patch is correct?
Thanks,
Adrian
The text was updated successfully, but these errors were encountered: