Repository navigation
Conversation
Signed-off-by: Kevin Grigorenko <kevin.grigorenko@us.ibm.com>
jasonk000
left a comment
There was a problem hiding this comment.
Small changes here, and one bug. Direction is good.
| @Argument(isMandatory = true) | ||
| public int number = 10000; | ||
|
|
||
| public enum SamplingType |
There was a problem hiding this comment.
I like this approach of allowing many sample types.
| finalObjectIds = Arrays.stream(objectIds).boxed().sorted((x, y) -> { | ||
| try | ||
| { | ||
| return Long.valueOf(snapshot.getRetainedHeapSize(y)).compareTo(snapshot.getRetainedHeapSize(x)); |
There was a problem hiding this comment.
To avoid extra boxing, we can use the form:
return Long.compare(snapshot.getRetainedHeapSize(y), snapshot.getRetainedHeapSize(x));
| public SamplingType sample = SamplingType.RANDOM; | ||
|
|
||
| @Argument(isMandatory = true) | ||
| public int number = 10000; |
There was a problem hiding this comment.
Not sure if this is validated in UI? I think it needs to be checked as >= 0.
| else if (sample == SamplingType.LAST) | ||
| { | ||
| finalObjectIds = new int[number]; | ||
| System.arraycopy(objectIds, objectIds.length - number - 1, finalObjectIds, 0, number); |
There was a problem hiding this comment.
This looks like an off by one, eg:
[5 6 7 8], length = 4, number = 1, target = 3.
System.arraycopy(objectIds, objectIds.length - number - 1, finalObjectIds, 0, number);
System.arraycopy(objectIds, 4 - 1 - 1, finalObjectIds, 0, 1);
System.arraycopy(objectIds, 2, finalObjectIds, 0, 1);
So I think this will get us [7], but I expect we want [8]?
| int[] objectIds = objects.getIds(listener); | ||
|
|
||
| int[] finalObjectIds = null; | ||
| if (objectIds.length < number) |
There was a problem hiding this comment.
could this be <= ? e.g length = 0, number = 0 ;; or length = 5, number = 5, could all match
| } | ||
|
|
||
| @Test | ||
| public void sampleObjects() throws SnapshotException |
There was a problem hiding this comment.
Maybe we can add extra test for the LAST.
There was a problem hiding this comment.
And some more .. the '0', '-ve', and other boundary conditions.
| } | ||
| } | ||
|
|
||
| public IResult execute(IProgressListener listener) throws Exception |
There was a problem hiding this comment.
Could add a try/finally around the whole fn to unwrap and rethrow the wrapped SnapshotException? An extra method layer to wrap the query implementation can keep it tidy.
There was a problem hiding this comment.
I initially tried that but I was surprised to find it didn't work because getRetainedHeapSize and getHeapSize throw the checked exception SnapshotException but sorted doesn't throw that exception so handling outside of sorted doesn't work. I'm still a bit curmudgeony and haven't fully mentally migrated to closures in Java but this seems like a huge oversight in the language that could have made closures much cleaner.
There was a problem hiding this comment.
handling outside of sorted doesn't work
Good point!
huge oversight in the language that could have made closures much cleaner
Yes, checked exceptions are a headache. Basically we need to create a @FunctionalInterface single-method interface that has the SnapshotException in the signature.
(not critical for change but may be some inspiration!, LLM whipped it up)
@FunctionalInterface
private interface SnapshotLongFunction<T>
{
long applyAsLong(T value) throws SnapshotException;
}
private static <T> Comparator<T> comparingSnapshot(SnapshotLongFunction<T> function,
boolean reversed)
{
return (left, right) -> {
try
{
long l = function.applyAsLong(left);
long r = function.applyAsLong(right);
return reversed ? Long.compare(r, l) : Long.compare(l, r);
}
catch (SnapshotException e)
{
throw new RuntimeException(e);
}
};
}
Now we can call:
...
.sorted(comparingSnapshot(snapshot::getRetainedHeapSize, true))
There was a problem hiding this comment.
Interesting. Speaking of which, I'm not sure what the policy is for LLMs for Eclipse and/or MAT? I'm still a bit curmudgeonly with LLMs, too, so I haven't even looked into the policies yet
There was a problem hiding this comment.
Found Eclipse AI guidelines here: https://www.eclipse.org/projects/handbook/#genai
Signed-off-by: Kevin Grigorenko <kevin.grigorenko@us.ibm.com>
Fixes #197