Please Review Test Fix of Bug 7177045

[Brad Wetmore]

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