RFR (M): 7606282: tests on ReservedSpace/VirtualSpace

[Stefan Karlsson]

Hi Aleksey,

Adding hotspot-runtime-dev, since this is adding tests to test/runtime.

Inlined:

On 2013-10-24 16:00, aleksey.timofeev at oracle.com wrote:
> Thank you, Stefan, for review. I fixed up a little. Also I moved bug 
> this changeset refers to from INTJDK to JDK: JDK-8027237 
> <https://bugs.openjdk.java.net/browse/JDK-8027237>.
>
> I updated changeset:
> - Added comments to WB_StressVirtualSpaceResize method of whitebox
> - Dropped changes of comment in virtualspace.cpp
> - Extracted class in ReadFromNoaccessArea.java file
>
> Updated webrev: 
> http://cr.openjdk.java.net/~iignatyev/atimofeev/ResVirtSpaceTests/webrev.03/ 
> <http://cr.openjdk.java.net/%7Eiignatyev/atimofeev/ResVirtSpaceTests/webrev.03/>

http://cr.openjdk.java.net/~iignatyev/atimofeev/ResVirtSpaceTests/webrev.03/src/share/vm/prims/whitebox.cpp.udiff.html

The indentation in the HotSpot code is two spaces.

Could you fix the weird indentation in:

+    if (! (UseCompressedOops && rhs.base() != NULL &&
+                                      (Universe::narrow_oop_base() != NULL) &&
+                                      Universe::narrow_oop_use_implicit_null_checks())) {

Can you separate 'UseCompressedOops && _base' in the output?

+        tty->print_cr("WB_ReadFromNoaccessArea method is useless:\n "
+                "\t\"UseCompressedOops && _base != NULL\" is %d\n"
+                "\t\"Universe::narrow_oop_base() != NULL\" is %d\n"
+                "\t\"Universe::narrow_oop_use_implicit_null_checks()\" is %d",
+                UseCompressedOops && rhs.base() != NULL,
+                Universe::narrow_oop_base() != NULL,
+                Universe::narrow_oop_use_implicit_null_checks());




There are a couple of places where you mix types of different sizes. 
Some examples:

%d is for int, but you dereference a char*:

+    tty->print_cr("*(vs.low_boundary() - rhs.noaccess_prefix() / 2 ) = 
%d",
+            *(vs.low_boundary() - rhs.noaccess_prefix() / 2 ));


magnitude is a jlong (8 bytes) and os::random() returns a long, which is 
platform dependent:

+        x = (os::random() % (2 * magnitude)) - magnitude;


%d is int, but seed is long:

+    tty->print_cr("Random seed is %d", seed);


Could you change x to be a size_t and widen the types where necessary, 
instead of shrinking them. For example:

+        if (x < 0 && (((long) vs.committed_size()) + x) < 0) {

Mixing int (%d) and long.

+    tty->print_cr("reservedSpaceSize=%d, magnitude=%d, iterations=%d",
+            (long) reservedSpaceSize, (long) magnitude, (long) iterations);

Could you move the declaration of 'long x' into the for loop?

+    for (long x, i = 0; i < iterations; i++ ) {
+

Theres a spurious semicolon:

+        } else {
+            vs.shrink_by(-x);
+        };

thanks,
StefanK

>
> On 10/20/2013 12:54 AM, aleksey.timofeev at oracle.com wrote:
>> Hello.
>>
>> Probably, I didn't get responses for my previous letter because I 
>> sent it with wrong subject.
>>
>> Could somebody review my changeset containing new tests against 
>> ReservedSpace and VirtualSpace classes? Please find webrev here: 
>> http://cr.openjdk.java.net/~iignatyev/atimofeev/ResVirtSpaceTests/webrev.02/ 
>> <http://cr.openjdk.java.net/%7Eiignatyev/atimofeev/ResVirtSpaceTests/webrev.02/>. 
>> Apart from feedback commit sponsorship wanted.
>>
>
> -- 
> Best regards, Aleksey Timofeev.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131025/5351d74e/attachment.htm>