Last Comment Bug 736555 - don't use magic values to implement deleted args
: don't use magic values to implement deleted args
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 659577
  Show dependency treegraph
 
Reported: 2012-03-16 11:53 PDT by Luke Wagner [:luke]
Modified: 2012-03-21 12:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (27.18 KB, patch)
2012-03-16 11:53 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (2) (32.11 KB, patch)
2012-03-16 15:19 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
hoisted bit functions (71 bytes, patch)
2012-03-19 10:48 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
hoisted bit functions (2.31 KB, patch)
2012-03-19 12:13 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-03-16 11:53:00 PDT
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.
Comment 1 Luke Wagner [:luke] 2012-03-16 15:19:17 PDT
Created attachment 606756 [details] [diff] [review]
patch (2)

Missed a spot
Comment 2 Brian Hackett (:bhackett) 2012-03-18 06:49:54 PDT
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] 2012-03-18 20:14:20 PDT
Good points, will do.
Comment 4 Luke Wagner [:luke] 2012-03-19 10:48:12 PDT
Created attachment 607214 [details] [diff] [review]
hoisted bit functions
Comment 5 Luke Wagner [:luke] 2012-03-19 12:13:19 PDT
Created attachment 607247 [details] [diff] [review]
hoisted bit functions

Non-empty patch this time
Comment 7 Matt Brubeck (:mbrubeck) 2012-03-21 12:01:18 PDT
https://hg.mozilla.org/mozilla-central/rev/537c3637b992

Note You need to log in before you can comment on or make changes to this bug.