[prev in list] [next in list] [prev in thread] [next in thread]
List: mono-devel-list
Subject: Re: [Mono-dev] How To Convince Mono To Allocate Big Arrays
From: "Rodrigo Kumpera" <kumpera () gmail ! com>
Date: 2008-05-28 17:53:52
Message-ID: 8cca42d80805281053q69b0ffc7te1ae2c61c3cd6e8 () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Luis,
I have committed your patch with some changes or missing parts. Thanks for
the contribuitins. Some of the comments were due to a more detailed review
while merging your code in.
Index: mono/metadata/object.c
===================================================================
--- mono/metadata/object.c (revision 17)
+++ mono/metadata/object.c (working copy)
@@ -3513,12 +3513,23 @@
}
/* helper macros to check for overflow when calculating the size of arrays
*/
-#define MYGUINT32_MAX 4294967295U
+#if (GLIB_SIZEOF_SIZE_T < 4 )
I changed the check to be against MONO_BIG_ARRAYS.
@@ -3548,34 +3559,32 @@
/* A single dimensional array with a 0 lower bound is the same as an
szarray */
....
if (CHECK_ADD_OVERFLOW_UN (byte_len, 3))
- mono_gc_out_of_memory (MYGUINT32_MAX);
+ mono_gc_out_of_memory (MONO_ARRAY_MAX_SIZE);
byte_len = (byte_len + 3) & ~3;
You might need to adjust this alignment check to be aware of your changes.
===================================================================
--- mono/metadata/icall.c (revision 17)
+++ mono/metadata/icall.c (working copy)
@@ -527,11 +527,11 @@
aklass = mono_bounded_array_class_get (mono_class_from_mono_type
(type->type), mono_array_length (lengths), bounded);
- sizes = alloca (aklass->rank * sizeof(guint32) * 2);
+ sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2);
for (i = 0; i < aklass->rank; ++i) {
- sizes [i] = mono_array_get (lengths, guint32, i);
+ sizes [i] = mono_array_get (lengths, gint32, i);
if (bounds)
- sizes [i + aklass->rank] = mono_array_get (bounds, guint32, i);
+ sizes [i + aklass->rank] = mono_array_get (bounds, gint32, i);
else
sizes [i + aklass->rank] = 0;
}
Why are you using mono_array_get with gint32? I can't think of a reason for
this change, as it won't matter for mono_array_new_full. Besides that, this
is asking for trouble as you are mixing signed and unsigned numbers.
@@ -560,6 +606,27 @@
mono_raise_exception (mono_get_exception_index_out_of_range ());
if (this->bounds == NULL)
+ length = this->max_length;
+ else
+ length = this->bounds [dimension].length;
+
+ if (length > G_MAXINT32)
+ mono_raise_exception (mono_get_exception_overflow ());
+
+ return length;
+}
I have put the length check inside a #ifdef MONO_BIG_ARRAYS
@@ -541,6 +541,51 @@
return array;
}
+static MonoArray *
+ves_icall_System_Array_CreateInstanceImpl64 (MonoReflectionType *type,
MonoArray *lengths, MonoArray *bounds)
...
+ for (i = 0; i < mono_array_length (lengths); i++)
+ if ((mono_array_get (lengths, gint64, i) < 0) ||
+ (mono_array_get (lengths, gint64, i) > MONO_ARRAY_MAX_INDEX))
+ mono_raise_exception (mono_get_exception_argument_out_of_range
(NULL));
Can't this expression be simplified to "if ((mono_array_get (lengths,
guint64, i) > MONO_ARRAY_MAX_INDEX)",
note we are reading it as an unsigned value.
...
+ sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * 2);
+ for (i = 0; i < aklass->rank; ++i) {
+ sizes [i] = mono_array_get (lengths, gint64, i);
+ if (bounds)
+ sizes [i + aklass->rank] = mono_array_get (bounds, gint64, i);
+ else
+ sizes [i + aklass->rank] = 0;
+ }
Note that sizes element type is an unsigned number, so we must read it
acordingly and cast it to mono_array_size_t to
ajust for the desired size - and avoid warnings as well.
We can use the same code for the bound check I suggested for both
CreateInstanceImpl versions. If we do that we could fix
the ugly aspect of having two 99% identical functions by defining it as a
MACRO and expanding it twice.
On Fri, May 23, 2008 at 7:10 PM, Luis F. Ortiz <LuisOrtiz@verizon.net>
wrote:
> Rodrigo:
> Sorry for the delayed response, things got busy, and I did not notice
> your message.
> Please go ahead and check in the parts that you feel most comfortable with,
> since I do not
> have commit rights. I'll adapt any further submissions against the next
> official cut.
>
> The changes I have made are to be released under the X11 license and I
> vouch that
> I have talked to the appropriate people at my employer (Interactive
> Supercomputing) and
> they agree as well to release this and any further contributions I make to
> Mono while
> under their employ under the X11 license. There, that ought to make
> everybody happy.
>
> Luis F. Ortiz
>
>
> On May 16, 2008, at 10:53 AM, Rodrigo Kumpera wrote:\
>
> Hi Luis,
>
> Most parts of your patch are ok to be commited. As I said before, it's
> better to have separate patches to speed up the review. As you get more
> changes in, it will ease your work on preparing patches and will ease on
> tracking head changes.
>
> Right now I'm ok with the configure, mono_array_t and related changes.
> These were easy to review and don't introduce any change in behavior, so we
> can check-in then right now. The others requires more testing from me and
> would be easier to both of us if done after.
> Do you mind cooking you a patch only with the ok parts.
>
> Remember that you need to either release these changes under the X11
> license or have done some copyright assignment paperwork.
>
> Thanks,
> Rodrigo
>
>
>
> On Thu, May 8, 2008 at 8:21 PM, Luis F. Ortiz <LuisOrtiz@verizon.net>
> wrote:
>
>> On May 8, 2008, at 9:29 AM, Rodrigo Kumpera wrote:
>>
>>> One important thing I forgot. If you break your patch into a few smaller
>>> ones the review process will be a lot easier to every one involved.
>>> The first one can introduce new types and configuration foo; then other
>>> to fix codegen and Array methods and; at last, a bunch of fixes to classlib
>>> issues -like sockets, file i/o and so on.
>>>
>>> Cheers,
>>> Rodrigo
>>>
>>
>> OK, here is another stab at the changes.
>>
>> This modifies the following files:
>> configure.in
>> mono/metadata/object.c
>> mono/metadata/object.h
>> mono/metadata/icall-def.h
>> mono/metadata/icall.c
>> mono/metadata/socket-io.c
>>
>> These files:
>> 1) Add a new configuration option, --enable-big-arrays, which defines
>> conditionally defines MONO_BIG_ARRAYS in config.h,
>> 2) Add in mono/metadata/object.h :
>> A) A new typedef for mono_array_size_t to be either guint32 or guint64
>> B) A new #define for MONO_ARRAY_MAX_INDEX for the biggest valid array
>> index, i.e. G_MAXINTxx
>> C) A new #define for MONO_ARRAY_MAX_SIZE for the biggest valid array
>> allocation, i.e. G_MAXUINTxx
>> D) The above all controlled by MONO_BIG_ARRAYS
>> 3) Modify the definitions of mono_array_new(), mono_array_new_full(), and
>> mono_array_new_specific() to match,
>> 4) Modify the usages of those functions in the metadata directory to
>> match,
>> 5) Add range checking in ves_icall_System_Array_CreateInstanceImpl64 so
>> it works with or without MONO_BIG_ARRAYS,
>> 6) Attempt to address all the mistakes you pointed out.
>>
>> These changes, in addition to the other changes I made, pass "make check"
>> with the exception of Tests.test_0_regress_75832(), as previously confessed.
>>
>>
>>
>>
>>
>> /Ortiz/Luis
>>
>>
>>
>
>
[Attachment #5 (text/html)]
Luis,<br><br>I have committed your patch with some changes or missing parts. Thanks \
for the contribuitins. Some of the comments were due to a more detailed review while \
merging your code in.<br><br><br>Index: mono/metadata/object.c<br> \
===================================================================<br>
--- mono/metadata/object.c (revision 17)<br>+++ \
mono/metadata/object.c (working copy)<br>@@ -3513,12 +3513,23 \
@@<br> }<br> <br> /* helper macros to check for overflow when \
calculating the size of arrays */<br>-#define MYGUINT32_MAX 4294967295U<br>
+#if (GLIB_SIZEOF_SIZE_T < 4 )<br><br>I changed the check to be against \
MONO_BIG_ARRAYS.<br><br>@@ -3548,34 +3559,32 @@<br> /* A \
single dimensional array with a 0 lower bound is the same as an szarray \
*/<br>....<br>
if (CHECK_ADD_OVERFLOW_UN (byte_len, \
3))<br>- \
mono_gc_out_of_memory (MYGUINT32_MAX);<br>+ \
mono_gc_out_of_memory \
(MONO_ARRAY_MAX_SIZE);<br> byte_len = \
(byte_len + 3) & ~3;<br><br>
You might need to adjust this alignment check to be aware of your \
changes.<br><br>===================================================================<br>--- \
mono/metadata/icall.c (revision 17)<br>
+++ mono/metadata/icall.c (working copy)<br>
@@ -527,11 +527,11 @@<br> <br> aklass = \
mono_bounded_array_class_get (mono_class_from_mono_type (type->type), \
mono_array_length (lengths), bounded);<br> <br>- sizes = \
alloca (aklass->rank * sizeof(guint32) * 2);<br>
+ sizes = alloca (aklass->rank * sizeof(mono_array_size_t) * \
2);<br> for (i = 0; i < aklass->rank; ++i) \
{<br>- sizes [i] = mono_array_get (lengths, \
guint32, i);<br>+ sizes [i] = mono_array_get \
(lengths, gint32, i);<br>
if (bounds)<br>- \
sizes [i + aklass->rank] = mono_array_get \
(bounds, guint32, i);<br>+ \
sizes [i + aklass->rank] = mono_array_get (bounds, gint32, \
i);<br> else<br> \
sizes [i + aklass->rank] = 0;<br>
}<br><br><br>Why are you using mono_array_get with gint32? I \
can't think of a reason for this change, as it won't matter for \
mono_array_new_full. Besides that, this is asking for trouble as you are mixing \
signed and unsigned numbers.<br>
<br><br> @@ -560,6 +606,27 @@<br> \
mono_raise_exception (mono_get_exception_index_out_of_range \
());<br> <br> if (this->bounds == \
NULL)<br>+ length = \
this->max_length;<br>+ else<br>+ \
length = this->bounds [dimension].length;<br>
+<br>+ if (length > G_MAXINT32)<br>+ \
mono_raise_exception \
(mono_get_exception_overflow ());<br>+<br>+ return \
length;<br>+}<br><br>I have put the length check inside a #ifdef \
MONO_BIG_ARRAYS<br><br><br>
<br>@@ -541,6 +541,51 @@<br> return \
array;<br> }<br> <br>+static MonoArray \
*<br>+ves_icall_System_Array_CreateInstanceImpl64 (MonoReflectionType *type, \
MonoArray *lengths, MonoArray *bounds)<br>...<br>+ for (i = 0; i \
< mono_array_length (lengths); i++) <br>
+ if ((mono_array_get (lengths, gint64, i) < \
0) ||<br>+ (mono_array_get \
(lengths, gint64, i) > MONO_ARRAY_MAX_INDEX))<br>+ \
mono_raise_exception \
(mono_get_exception_argument_out_of_range (NULL));<br>
<br>Can't this expression be simplified to "if ((mono_array_get (lengths, \
guint64, i) > MONO_ARRAY_MAX_INDEX)",<br>note we are reading it as an \
unsigned value.<br>...<br>+ sizes = alloca (aklass->rank * \
sizeof(mono_array_size_t) * 2);<br>
+ for (i = 0; i < aklass->rank; ++i) {<br>+ \
sizes [i] = mono_array_get (lengths, gint64, \
i);<br>+ if (bounds)<br>+ \
sizes [i + aklass->rank] = mono_array_get \
(bounds, gint64, i);<br>+ else<br>
+ sizes [i + aklass->rank] \
= 0;<br>+ }<br><br>Note that sizes element type is an unsigned \
number, so we must read it acordingly and cast it to mono_array_size_t to<br>ajust \
for the desired size - and avoid warnings as well.<br>
<br>We can use the same code for the bound check I suggested for both \
CreateInstanceImpl versions. If we do that we could fix<br>the ugly aspect of having \
two 99% identical functions by defining it as a MACRO and expanding it twice.<br>
<br><br><br>
<div class="gmail_quote">On Fri, May 23, 2008 at 7:10 PM, Luis F. Ortiz <<a \
href="mailto:LuisOrtiz@verizon.net" target="_blank">LuisOrtiz@verizon.net</a>> \
wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, \
204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div><div>Rodrigo:</div><div> Sorry for the delayed response, \
things got busy, and I did not notice your message.</div><div>Please go ahead and \
check in the parts that you feel most comfortable with, since I do not</div>
<div>have commit rights. I'll adapt any further submissions \
against the next official cut. </div><div><br></div><div> The \
changes I have made are to be released under the X11 license and I vouch \
that</div><div>I have talked to the appropriate people at my employer (Interactive \
Supercomputing) and</div>
<div>they agree as well to release this and any further contributions I make to Mono \
while </div><div>under their employ under the X11 license. There, \
that ought to make everybody happy.</div><div><br></div><div>Luis F. Ortiz</div>
<div><br></div><div><br></div><div>On May 16, 2008, at 10:53 AM, Rodrigo Kumpera \
wrote:\</div><div><div></div><div><blockquote type="cite">Hi Luis,<br><br>Most parts \
of your patch are ok to be commited. As I said before, it's better to have \
separate patches to speed up the review. As you get more changes in, it will ease \
your work on preparing patches and will ease on tracking head changes.<br>
<br>Right now I'm ok with the configure, mono_array_t and related changes. These \
were easy to review and don't introduce any change in behavior, so we can \
check-in then right now. The others requires more testing from me and would be easier \
to both of us if done after. <br>
Do you mind cooking you a patch only with the ok parts.<br><br>Remember that you \
need to either release these changes under the X11 license or have done some \
copyright assignment paperwork.<br><br>Thanks,<br>Rodrigo<br>
<br>
<br><br><div class="gmail_quote">On Thu, May 8, 2008 at 8:21 PM, Luis F. Ortiz \
<<a href="mailto:LuisOrtiz@verizon.net" \
target="_blank">LuisOrtiz@verizon.net</a>> wrote:<br><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;">
<div>On May 8, 2008, at 9:29 AM, Rodrigo Kumpera wrote:<br> <blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> One important thing I forgot. If you break your patch \
into a few smaller ones the review process will be a lot easier to every one \
involved.<br>
The first one can introduce new types and configuration foo; then other to fix \
codegen and Array methods and; at last, a bunch of fixes to classlib issues -like \
sockets, file i/o and so on.<br> <br> Cheers,<br> Rodrigo<br>
</blockquote> <br></div> OK, here is another stab at the changes.<br> <br> This \
modifies the following files:<br> <a \
href="http://configure.in" target="_blank">configure.in</a><div><br> \
mono/metadata/object.c<br>
mono/metadata/object.h<br> \
mono/metadata/icall-def.h<br> \
mono/metadata/icall.c<br> \
mono/metadata/socket-io.c<br> <br></div> These files:<br> 1) Add a new \
configuration option, --enable-big-arrays, which defines conditionally defines \
MONO_BIG_ARRAYS in config.h,<br>
2) Add in mono/metadata/object.h :<br> A) A new typedef for \
mono_array_size_t to be either guint32 or guint64<br> B) A new #define for \
MONO_ARRAY_MAX_INDEX for the biggest valid array index, i.e. G_MAXINTxx<br> C) \
A new #define for MONO_ARRAY_MAX_SIZE for the biggest valid array allocation, i.e. \
G_MAXUINTxx<br>
D) The above all controlled by MONO_BIG_ARRAYS<br> 3) Modify the \
definitions of mono_array_new(), mono_array_new_full(), and mono_array_new_specific() \
to match,<br> 4) Modify the usages of those functions in the metadata directory \
to match,<br>
5) Add range checking in ves_icall_System_Array_CreateInstanceImpl64 so it \
works with or without MONO_BIG_ARRAYS,<br> 6) Attempt to address all the \
mistakes you pointed out.<br> <br> These changes, in addition to the other changes I \
made, pass "make check" with the exception of Tests.test_0_regress_75832(), \
as previously confessed.<br>
<br> <br><br> <br> <br> /Ortiz/Luis<br> <br> \
<br></blockquote></div><br></blockquote></div></div></div><br></div></blockquote></div><br>
_______________________________________________
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic