Please Review Test Fix of Bug 7177045 (original) (raw)

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