The assert of a fast case array should be a check. Otherwise LGTM.
On Tue, Sep 9, 2008 at 9:09 AM, <[EMAIL PROTECTED]> wrote: > Erik, > > I'd like you to do a code review. To review this change, run > > gvn review --project https://v8.googlecode.com/svn [EMAIL PROTECTED]/[EMAIL > PROTECTED] > > Alternatively, to review the latest snapshot of this change > branch, run > > gvn --project https://v8.googlecode.com/svn review [EMAIL > PROTECTED]/faster-contains-check > > to review the following change: > > [EMAIL PROTECTED]/[EMAIL PROTECTED] | [EMAIL PROTECTED] | 2008-09-09 08:06:57 > +-100 (Tue, 09 Sep 2008) > > Description: > > Move the contains check in array join from javascript to C++. > > > > Affected Paths: > M //branches/bleeding_edge/src/array.js > M //branches/bleeding_edge/src/runtime.cc > M //branches/bleeding_edge/src/runtime.h > > > This is a semiautomated message from "gvn mail". See > <http://code.google.com/p/gvn/> to learn more. > > Index: src/array.js > =================================================================== > --- src/array.js (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) > +++ src/array.js (^/changes/[EMAIL > PROTECTED]/faster-contains-check/bleeding_edge/src/[EMAIL PROTECTED]) > @@ -111,8 +111,7 @@ function Join(array, length, separator, convert) { > if (is_array) { > // If the array is cyclic, return the empty string for already > // visited arrays. > - if (Contains(visited_arrays, array)) return ''; > - visited_arrays[visited_arrays.length] = array; > + if (!%PushIfAbsent(visited_arrays, array)) return ''; > } > > // Attempt to convert the elements. > @@ -702,17 +701,17 @@ function ArraySort(comparefn) { > while (true) { > var child_index = ((parent_index + 1) << 1) - 1; > if (child_index >= i) break; > - var child1_value = this[child_index]; > + var child1_value = this[child_index]; > var child2_value = this[child_index + 1]; > var parent_value = this[parent_index]; > if (child_index + 1 >= i || Compare(child1_value, child2_value) > 0) { > if (Compare(parent_value, child1_value) > 0) break; > - this[child_index] = parent_value; > + this[child_index] = parent_value; > this[parent_index] = child1_value; > parent_index = child_index; > } else { > if (Compare(parent_value, child2_value) > 0) break; > - this[child_index + 1] = parent_value; > + this[child_index + 1] = parent_value; > this[parent_index] = child2_value; > parent_index = child_index + 1; > } > Index: src/runtime.cc > =================================================================== > --- src/runtime.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) > +++ src/runtime.cc (^/changes/[EMAIL > PROTECTED]/faster-contains-check/bleeding_edge/src/[EMAIL PROTECTED]) > @@ -3443,6 +3443,25 @@ static Object* Runtime_SetNewFunctionAttributes(Ar > } > > > +// Push an array unto an array of arrays if it is not already in the > +// array. Returns true if the element was pushed on the stack and > +// false otherwise. > +static Object* Runtime_PushIfAbsent(Arguments args) { > + ASSERT(args.length == 2); > + CONVERT_CHECKED(JSArray, array, args[0]); > + CONVERT_CHECKED(JSArray, element, args[1]); > + ASSERT(array->HasFastElements()); > + int length = Smi::cast(array->length())->value(); > + FixedArray* elements = FixedArray::cast(array->elements()); > + for (int i = 0; i < length; i++) { > + if (elements->get(i) == element) return Heap::false_value(); > + } > + Object* obj = array->SetFastElement(length, element); > + if (obj->IsFailure()) return obj; > + return Heap::true_value(); > +} > + > + > // This will not allocate (flatten the string), but it may run > // very slowly for very deeply nested ConsStrings. For debugging use only. > static Object* Runtime_GlobalPrint(Arguments args) { > Index: src/runtime.h > =================================================================== > --- src/runtime.h (^/branches/bleeding_edge/src/[EMAIL PROTECTED]) > +++ src/runtime.h (^/changes/[EMAIL > PROTECTED]/faster-contains-check/bleeding_edge/src/[EMAIL PROTECTED]) > @@ -61,6 +61,9 @@ namespace v8 { namespace internal { > F(LazyCompile, 1) \ > F(SetNewFunctionAttributes, 1) \ > \ > + /* Array join support */ \ > + F(PushIfAbsent, 2) \ > + \ > /* ConsStrings */ \ > F(ConsStringFst, 1) \ > F(ConsStringSnd, 1) \ > > -- Erik Corry, Software Engineer Google Denmark ApS. CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
