1282104 - jsarray.cpp's SetArrayElement sometimes defines instead

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

It doesn't just set an element, it doesn't just define an element, it sometimes does one, sometimes does the other, even tho all callers require exactly one of the two behaviors.  An example case that's currently buggy:

  try
  {
    Object.defineProperty(Array.prototype, 0,
                          { configurable: true,
                            set: function() { throw 42; } });
    [].push(5);
    throw new Error("didn't throw");
  }
  catch (e)
  {
    assertEq(e, 42);
  }

If bug 1163091 comment 6 is to be believed, there are about 19 separate places that need auditing, each requiring switching to a setting-functionality function or a defining-functionality function.  Someone should introduce setting-only and defining-only functions, then individually switch each of those 19 users to the appropriate thing, then kill SetArrayElement completely.

Comment 1

[Jason Orendorff [:jorendorff]]

I reviewed every call to SetArrayElement. All of them want to set (not define) properties.

I also reviewed every call to InitArrayElements. Its 3 callers all want to set (not define) properties.

All that remains is to review SetOrExtendBoxedOrUnboxedDenseElements in vm/UnboxedObject-inl.h for correctness, and add checks for any additional situations in which it should return DenseElementResult::Incomplete.

Comment 3

[Jan de Mooij [:jandem]]

Is this still on your radar? Based on comment 1 it seems like this shouldn't be too hard to clean up and it would fix a number of spec issues, like bug 1313891.

Comment 4

[André Bargull [:anba]]

Attachment: bug1282104-part1.patch

First some clean-up work:
- DoGetElement and GetElement were always called with uint32_t indices, so we don't need the IndexType template parameter
- AssertGreaterThanZero is always called with uint32_t indices, but that's a no-op, so we can just remove the complete function
- Renamed InitArrayElements to SetArrayElements, so it's less confusing when compared to the init(Dense)Elements methods on boxed and unboxed arrays
- Removed the fallible call to IsArray() in array_push() with the new IsBoxedOrUnboxedArray function, because we don't actually need to handle proxies at this point
- Changed some index types from double to uint32_t to avoid implicit coercion when calling GetElement
- Removed the |receiver| parameter from SliceSlowly, because that method is always called with |obj == receiver|

Comment 5

[André Bargull [:anba]]

Attachment: bug1282104-part2.patch

I've removed the SetOrExtendAnyBoxedOrUnboxedDenseElements fast path from SetArrayElement, because that method is (most of the time) only called when previous fast paths failed (cf. SetArrayElements, array_reverse, array_shift, array_unshift, array_splice_impl).

There are three exceptions which were updated as follows:
- array_sort: I've added the new FillWithUndefined method to create a new fast path to fill an array with undefined values. (Is there are better way to do this to avoid calling SetOrExtendAnyBoxedOrUnboxedDenseElements repeatedly?)
- array_splice_impl: The final loop in that method now calls SetArrayElements, which in turn has a fast path for dense arrays.
- array_unshift: Unboxed arrays now take the slow path, because the previous fast path explicitly excluded unboxed arrays, but there's actually no reason why unboxed arrays can't be optimized here. I've added a todo note to fix this issue in a follow-up bug.

And finally, because SetArrayElement no longer contains any special cases for (un)boxed arrays, I've renamed the method to SetElement.

The alternative to this approach is guarding the fast path in Set(Array)Element with ObjectMayHaveExtraIndexedProperties. But unless Talos regressions show up, I'm in favour of keeping the simpler approach instead of checking ObjectMayHaveExtraIndexedProperties when we're already in a slow path.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8818977 [details] [diff] [review]
bug1282104-part1.patch

Review of attachment 8818977 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice, thanks.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8818979 [details] [diff] [review]
bug1282104-part2.patch

Review of attachment 8818979 [details] [diff] [review]:
-----------------------------------------------------------------

Great to have this fixed.

::: js/src/jsarray.cpp
@@ +1858,5 @@
> +            if (!CheckForInterrupt(cx))
> +                return false;
> +
> +            DenseElementResult result =
> +                SetOrExtendAnyBoxedOrUnboxedDenseElements(cx, obj, startIndex, v.address(), 1);

Unboxed arrays can't store |undefined|, so this will always return Incomplete and we will break out of the loop. Considering that, it might be simpler and faster to optimize for native objects only. Something like

  result = obj->ensureDenseElements(cx, startIndex, endIndex - startIndex);
  ...
  while (startIndex < endIndex)
      obj->setDenseElement(startIndex++, v);

(And add a brief comment mentioning this.)

@@ +2315,5 @@
>          if (length > 0) {
>              // Only include a fast path for boxed arrays. Unboxed arrays can't
>              // be optimized here because unshifting temporarily places holes at
>              // the start of the array.
> +            // TODO: Implement unboxed array optimization similar to the one in

Unboxed arrays are not enabled by default and we don't have anyone working on that atm, so it seems fine to defer to a follow-up.

Comment 8

[André Bargull [:anba]]

Attachment: bug1282104-part1.patch

Update part 1 to apply cleanly on inbound. Carrying r+ from jandem.

Comment 9

[André Bargull [:anba]]

Attachment: bug1282104-part2.patch

Updated part 2 per review comment.

Re-requesting review because I'm now wondering if it makes sense to move parts of the new FillWithUndefined function into UnboxedObject-inl.h, so it's more closely located to the code from which I copied parts of FillWithUndefined: https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/js/src/vm/UnboxedObject-inl.h#481-503

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8830299 [details] [diff] [review]
bug1282104-part2.patch

Review of attachment 8830299 [details] [diff] [review]:
-----------------------------------------------------------------

I think it makes sense to keep FillWithUndefined in jsarray.cpp, where it's used. If the caller disappears for some reason, we can remove that function.

FillWithUndefined is not very hot right? We could optimize the setDenseElementWithType calls a little (after the first call we know |undefined| is part of the element types), but it's probably not worth it.

::: js/src/jsarray.cpp
@@ +1884,5 @@
> +            obj->as<ArrayObject>().setLengthInt32(start + count);
> +
> +        for (uint32_t i = 0; i < count; i++)
> +            nobj->setDenseElementWithType(cx, start + i, UndefinedHandleValue);
> +        optimized = true;

I'd prefer removing this optimized bool and just returning true here.

Comment 11

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> FillWithUndefined is not very hot right? We could optimize the
> setDenseElementWithType calls a little (after the first call we know
> |undefined| is part of the element types), but it's probably not worth it.

I don't think so - actually the new code makes it even faster when the dense array optimization in FillWithUndefined can be used.

Comment 12

[André Bargull [:anba]]

Attachment: bug1282104-part2.patch

Updated part 2 per review comments. Carrying r+ from jandem.

Comment 13

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8108f3ad4aace2f8454fae063768e8a6078cdd2

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb28331a06a5
Part 1: Remove dead code in jsarray.cpp and avoid implicit type coercion from double to uint32. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aae92d8aa00
Part 2: Remove dense fast path in Array.prototype methods which didn't check for inherited accessors. r=jandem

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb28331a06a5
https://hg.mozilla.org/mozilla-central/rev/6aae92d8aa00