Please Review Test Fix of Bug 7177045
Brad Wetmore
bradford.wetmore at oracle.com
Thu Jul 12 23:22:10 UTC 2012
Sorry for the delay Dan. Please work with Kurchi to get this pushed.
On 7/5/2012 1:38 PM, Dan Xu wrote:
> Hi Brad,
>
> Thanks for your good suggestions. I have fixed most of them and
> re-uploaded my changes at http://cr.openjdk.java.net/~dxu/7177045.01/.
>
> The reason that I chose ArrayDeque instead of LinkedList is that
> ArrayDequeseems have better performance. According to the java doc,
> "most ArrayDeque operations run in amortized constant time" and "this
> class is likely to be faster then LinkedList when used as a queue." It
> is also very easy to remove last elements to back off memory allocation.
Great, thanks for the pointer.
> In addition, I did not switch to diamond operator. Because old Jdk
> bundles, say jdk 1.7.0-ea-b23 and jdk 1.7.0-ea-b29 used in my testing,
> failed to compile diamond operator. Here are the compilation error messages,
>
> TestProviderLeak.java:62: illegal start of type
> Deque<byte []> data = new ArrayDeque<>();
> ^
> 1 error
>
> I guess those jdk might be too early to adopt the diamond operator
> changes. I am not sure whether we still take these old jdk bundles into
> consideration here. Thanks!
Yes, it would be good to actually show that it failed on b23 and not on
b29 without having to modify the source. Good point.
>> Minor nit: line 38 has a space at the end of the line. Current
>> jstyle guidelines state no indention with tabs and no whitespace at
>> the end of the lines.
Looks good.
>> Suggest more liberal use of comments, either in the method's comments
>> or inline. Good to explain your assumptions/approach in case things
>> aren't obvious. For example, why do you backoff 3MB after allocating
>> available memory? And at line 134: the operation could either time
>> out or threw an exception. Nice to make that clear.
Thanks!
>> Line 114: consider adding a @overrides annotation on the call() method.
Good.
>> Line 139: I'm being paranoid here, but shutdownNow doesn't guarantee
>> threads will be stopped. If we actually got into a situation where
>> there was a timeout, executor.shutdownNow() *may* never return. One
>> reason is it might be hanging somewhere waiting for memory. I would
>> suggest as part of your finally block, you dequeue all the memory in
>> dummyData, call System.gc(), then run executor.shutdownNow(). JTREG
>> will timeout after two minutes, but if we can proactively help the
>> situation, we might as well.
>>
>> Otherwise, looks good. We'll wait to see if anyone has other
>> thoughts, and if not, we'll push when you're back from vacation.
Thanks.
Brad
More information about the security-dev
mailing list