jsarray.cpp's SetArrayElement sometimes defines instead

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Waldo, Assigned: anba)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
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.
Summary: Kill jsarray.cpp's SetArrayElement → jsarray.cpp's SetArrayElement sometimes defines instead
Assignee: nobody → jorendorff
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1313891
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.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 4

a year ago
Created attachment 8818977 [details] [diff] [review]
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|
Attachment #8818977 - Flags: review?(jdemooij)
(Assignee)

Comment 5

a year ago
Created attachment 8818979 [details] [diff] [review]
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.
Attachment #8818979 - Flags: review?(jdemooij)
Comment on attachment 8818977 [details] [diff] [review]
bug1282104-part1.patch

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

Very nice, thanks.
Attachment #8818977 - Flags: review?(jdemooij) → review+
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.
Attachment #8818979 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 8

a year ago
Created attachment 8830296 [details] [diff] [review]
bug1282104-part1.patch

Update part 1 to apply cleanly on inbound. Carrying r+ from jandem.
Attachment #8818977 - Attachment is obsolete: true
Attachment #8830296 - Flags: review+
(Assignee)

Comment 9

a year ago
Created attachment 8830299 [details] [diff] [review]
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
Attachment #8818979 - Attachment is obsolete: true
Attachment #8830299 - Flags: review?(jdemooij)
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.
Attachment #8830299 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 11

a year ago
(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.
(Assignee)

Comment 12

a year ago
Created attachment 8831046 [details] [diff] [review]
bug1282104-part2.patch

Updated part 2 per review comments. Carrying r+ from jandem.
Attachment #8830299 - Attachment is obsolete: true
Attachment #8831046 - Flags: review+
(Assignee)

Updated

a year ago
Assignee: jorendorff → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(jorendorff)

Comment 14

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb28331a06a5
https://hg.mozilla.org/mozilla-central/rev/6aae92d8aa00
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1335619
Depends on: 1335623
You need to log in before you can comment on or make changes to this bug.