736555 - don't use magic values to implement deleted args

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

When there is an args object, bug 659577 wants their canonical location to be the arguments object.  The current strategy for handling deleted elements (storing a magic value) is a problem when the element is deleted from the arguments object.  E.g., in:

  function (x) { delete arguments[0]; print(x) }

we need to retain x's value in the arguments object while, at the same time, remembering that the element is deleted.  The solution in this patch is to store deleted-ness in a bitfield that hangs off ArgumentsData and never overwrite the actual formal argument's slot.

This changes the arguments ic.  However, after bhackett's 'arguments' analysis in analyzeSSA, it isn't necessary for perf anymore since arguments objects tend to not be created.  Rather than fix this doomed (b/c of IM) code, I just removed it.  V8/Dromaeo scores unaffected.

Comment 1

[Luke Wagner [:luke]]

Attachment: patch (2)

Missed a spot

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 606756 [details] [diff] [review]
patch (2)

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

::: js/src/vm/ArgumentsObject-inl.h
@@ +85,5 @@
>  
> +inline size_t
> +ArgumentsObject::numDeletedBitsWords(size_t length)
> +{
> +    return 1 + length / JS_BITS_PER_WORD;

Is there a good common header where this grubby bit math can go, and which ArgumentsObject can then wrap?  It also looks like this will overallocate for length % JS_BITS_PER_WORD == 0.

@@ +99,5 @@
> +ArgumentsObject::isElementDeleted(uint32_t i) const
> +{
> +    unsigned index = i / JS_BITS_PER_WORD;
> +    size_t mask = size_t(1) << (i % JS_BITS_PER_WORD);
> +    JS_ASSERT(index < numDeletedBitsWords(initialLength()));

Ditto.

@@ +118,5 @@
> +ArgumentsObject::markElementDeleted(uint32_t i)
> +{
> +    unsigned index = i / JS_BITS_PER_WORD;
> +    size_t mask = size_t(1) << (i % JS_BITS_PER_WORD);
> +    JS_ASSERT(index < numDeletedBitsWords(initialLength()));

Ditto.

Comment 3

[Luke Wagner [:luke]]

Good points, will do.

Comment 4

[Luke Wagner [:luke]]

Attachment: hoisted bit functions

Comment 5

[Luke Wagner [:luke]]

Attachment: hoisted bit functions

Non-empty patch this time

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/537c3637b992

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/537c3637b992