don't use magic values to implement deleted args

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 606663 [details] [diff] [review]
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.
Attachment #606663 - Flags: review?(bhackett1024)
(Assignee)

Comment 1

6 years ago
Created attachment 606756 [details] [diff] [review]
patch (2)

Missed a spot
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #606756 - Flags: review?(bhackett1024)
(Assignee)

Updated

6 years ago
Attachment #606663 - Attachment is obsolete: true
Attachment #606663 - Flags: review?(bhackett1024)
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.
Attachment #606756 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 3

6 years ago
Good points, will do.
(Assignee)

Comment 4

6 years ago
Created attachment 607214 [details] [diff] [review]
hoisted bit functions
Attachment #607214 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

6 years ago
Created attachment 607247 [details] [diff] [review]
hoisted bit functions

Non-empty patch this time
Attachment #607214 - Attachment is obsolete: true
Attachment #607214 - Flags: review?(bhackett1024)
Attachment #607247 - Flags: review?(bhackett1024)
(Assignee)

Updated

6 years ago
Attachment #607247 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/537c3637b992
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/537c3637b992
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.