836949 - e4x removal follow-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Things I didn't want to tackle in bug 788293:

- PNX_FUNCDEFS and PNX_SETCALL have the same value.

- All the version/flags stuff is over-engineered, now.  E.g.:
  - remove JSCOMPILEOPTION_MASK
  - remove FULL_MASK
  - remove |copts| in jscntxt.h
  - remove OptionFlagsToVersion
  - etc

- PushNodeChildren: in the PN_NULLARY case, just return true.

- make_unicode.py needs to be updated and re-run.  evilpie, can you do this?

Comment 1

[Tom S [:evilpie]]

Attachment: (part 2) - remove e4x only unicode flags

Comment 2

[Nicholas Nethercote [inactive]]

> - PushNodeChildren: in the PN_NULLARY case, just return true.

I got an assertion failure in jit-test/tests/auto-regress/bug601393.js with this change, so I won't do it.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Clean up the versions/options code.

"Runtime options" are now just "options", and "compile options" are gone.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Disambiguate PNX_FUNCDEFS and PNX_SETCALL.

Comment 5

[Jason Orendorff [:jorendorff]]

OK but I'm calling dibs on deleting the unused source notes.

Comment 6

[Nicholas Nethercote [inactive]]

> OK but I'm calling dibs on deleting the unused source notes.

Sure thing!  I didn't realize we had ones to delete.  I was going to ask you to CC me on the bug, but instead I just filed it myself:  bug 838813.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 710066 [details] [diff] [review]
(part 2) - Disambiguate PNX_FUNCDEFS and PNX_SETCALL.

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

According to bug 838813 comment 16, I have successfully ignored this long enough that I don't need to review it anymore.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 710064 [details] [diff] [review]
(part 1) - Clean up the versions/options code.

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

Very nice!

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08baa859a8d4

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08baa859a8d4

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 709747 [details] [diff] [review]
(part 2) - remove e4x only unicode flags

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

Whew, fun times paging all this code back into memory.

::: js/src/vm/make_unicode.py
@@ +211,5 @@
>   * Step 2:
>   *  Using these bits to get an reduced index from index1.
>   *   index = index1[upper]
>   * Step 3:
> + *  Combining the index and the bottom 5 bits of the orginial jschar.

original

@@ +217,2 @@
>   *
>   * The advantage here is that the bigest number in index1 doesn't need 10 bits,

biggest

@@ +217,3 @@
>   *
>   * The advantage here is that the bigest number in index1 doesn't need 10 bits,
> + * but 7 and we save some memory.

I find myself wondering if we shouldn't make this output be generated based on |shift| directly.

"""all that comment""".format(shift = shift, shiftComplementStars = "*" * (16 - shift), shiftDots = "." * shift)

and so on.  But I guess this suffices to make it correct, tho.

Comment 12

[Jeff Walden [:Waldo]]

For that matter, I'm thinking probably we should generate both Unicode.h and Unicode.cpp, so that we can just always use the computed shift value, and never hard-code it anywhere.  This hardcoding, I note, has the potential to result in some UnicodeData.txt update causing the update script to break, if enough characters of interest got added in the right ways.  I think.  Right?  New bug to follow up on that if you agree.

Comment 13

[Tom S [:evilpie]]

Attachment: (part 3) - remove InternNonIntElementId/FetchElementId

These all have an extra |obj| parameter and some really strange semantics that I would rather get rid of.

Comment 14

[Tom S [:evilpie]]

Attachment: (part 4) remove obj parameter from ValueToId

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 715554 [details] [diff] [review]
(part 4) remove obj parameter from ValueToId

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

Stealing review.

::: js/src/jsinterp.cpp
@@ +1590,5 @@
>      }
>  }
>  END_CASE(JSOP_AND)
>  
> +#define FETCH_ELEMENT_ID(cx, n, id)                                           \

The macros in jsinterp.cpp are mostly unhygienic.  So there's not much point passing in |cx| here -- I'd just remove it.

Comment 16

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9412ce386841

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9412ce386841

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 715553 [details] [diff] [review]
(part 3) - remove InternNonIntElementId/FetchElementId

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

r- for the BytecodeEmitter.cpp bit, otherwise fine enough.  Starting to use PropertyKey everywhere can't come soon enough!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3527,5 @@
> +                    return false;
> +                }
> +            }
> +
> +            JSAtom *name = ToAtom<CanGC>(cx, idvalue);

Erm, shouldn't everything from this ToAtom downward be in an else-block corresponding to the IsDefinitelyIndex call above?  If it's definitely an element, you're defining the element twice, no?

Comment 19

[Tom S [:evilpie]]

Attachment: (part 3) - remove InternNonIntElementId/FetchElementId !v2

Comment 20

[Tom S [:evilpie]]

Attachment: (part 4) remove obj parameter from ValueToId !v2

Carrying r+.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 730699 [details] [diff] [review]
(part 3) - remove InternNonIntElementId/FetchElementId  !v2

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3459,5 @@
>              if (!pn->pn_right->getConstantValue(cx, strictChecks, &value))
>                  return false;
>  
>              ParseNode *pnid = pn->pn_left;
> +            RootedValue idvalue(cx);

This is best put outside the loop, to avoid eventual push/pop overhead for hooking this up to the Rooted stack to track these.

@@ +3471,5 @@
> +
> +            uint32_t index;
> +            if (IsDefinitelyIndex(idvalue, &index)) {
> +                if (!JSObject::defineElement(cx, obj, index, value, NULL, NULL,
> +                                             JSPROP_ENUMERATE))

This looks like it'd fit in 100ch to me, but maybe you've actually measured.  :-)  Same for the other defineElement below.

::: js/src/jsatominlines.h
@@ +36,5 @@
> +    uint32_t index;
> +    if (atom->isIndex(&index) && index <= JSID_INT_MAX)
> +        return INT_TO_JSID((int32_t) index);
> +
> +    return JSID_FROM_BITS((size_t)atom);

Constructor-style casts here, please.

::: js/src/jsinterp.cpp
@@ +2936,5 @@
>  BEGIN_CASE(JSOP_INITELEM)
>  {
>      JS_ASSERT(regs.stackDepth() >= 3);
>      HandleValue val = HandleValue::fromMarkedLocation(&regs.sp[-1]);
> +    HandleValue id = MutableHandleValue::fromMarkedLocation(&regs.sp[-2]);

Not HandleValue::fromMarkedLocation?

Comment 22

[Tom S [:evilpie]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> Comment on attachment 730699 [details] [diff] [review]
> (part 3) - remove InternNonIntElementId/FetchElementId  !v2
> 
> Review of attachment 730699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +3459,5 @@
> >              if (!pn->pn_right->getConstantValue(cx, strictChecks, &value))
> >                  return false;
> >  
> >              ParseNode *pnid = pn->pn_left;
> > +            RootedValue idvalue(cx);
> 
> This is best put outside the loop, to avoid eventual push/pop overhead for
> hooking this up to the Rooted stack to track these.
> 
Good catch! I also moved |value| outside.
> @@ +3471,5 @@
> > +
> > +            uint32_t index;
> > +            if (IsDefinitelyIndex(idvalue, &index)) {
> > +                if (!JSObject::defineElement(cx, obj, index, value, NULL, NULL,
> > +                                             JSPROP_ENUMERATE))
> 
> This looks like it'd fit in 100ch to me, but maybe you've actually measured.
> :-)  Same for the other defineElement below.
> 
I made this change, however it looks kind of strange, because the defineElement doesn't fit.
> ::: js/src/jsatominlines.h
> @@ +36,5 @@
> > +    uint32_t index;
> > +    if (atom->isIndex(&index) && index <= JSID_INT_MAX)
> > +        return INT_TO_JSID((int32_t) index);
> > +
> > +    return JSID_FROM_BITS((size_t)atom);
> 
> Constructor-style casts here, please.
> 
> ::: js/src/jsinterp.cpp
> @@ +2936,5 @@
> >  BEGIN_CASE(JSOP_INITELEM)
> >  {
> >      JS_ASSERT(regs.stackDepth() >= 3);
> >      HandleValue val = HandleValue::fromMarkedLocation(&regs.sp[-1]);
> > +    HandleValue id = MutableHandleValue::fromMarkedLocation(&regs.sp[-2]);
> 
> Not HandleValue::fromMarkedLocation?
Hehe.

Comment 23

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53273047c43e
https://hg.mozilla.org/integration/mozilla-inbound/rev/471f0b0a58e1

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53273047c43e
https://hg.mozilla.org/mozilla-central/rev/471f0b0a58e1

Comment 25

[Tom S [:evilpie]]

Attachment: (part 5) Remove GetMethod

We used to have some special behavior when you wanted to get property as a method from e4x objects. This is now obviously gone.

I don't really have anything on my mind that would be easy to fix now. Any ideas left?

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 742556 [details] [diff] [review]
(part 5) Remove GetMethod

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

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +1266,1 @@
>              // XXX We really want to factor out the error reporting better and

Why is this comment moving?  It looks like it applied to what happened if !JS_GetMethod(...), which would now be !JS_GetProperty(...), which would mean it shouldn't move, and the |thisObj = obj| assignment should be after it.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 742556 [details] [diff] [review]
(part 5) Remove GetMethod

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

::: js/src/json.cpp
@@ +270,5 @@
>  
>      /* Step 2. */
>      if (vp.get().isObject()) {
>          RootedValue toJSON(cx);
> +        RootedObject obj(cx, &vp.get().toObject());

Do we really need those .get()s?

Comment 28

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6765e6e3be

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f6765e6e3be

Comment 30

[Jeff Walden [:Waldo]]

(In reply to Tom Schuster [:evilpie] from comment #25)
> I don't really have anything on my mind that would be easy to fix now. Any
> ideas left?

Nothing in particular springs to mind.

(In reply to :Ms2ger from comment #27)
> > +        RootedObject obj(cx, &vp.get().toObject());
> 
> Do we really need those .get()s?

No.  Possibly they precede our adding the Value interface bits to {Mutable,}Handle<Value>.

Comment 31

[Tom S [:evilpie]]

OK. I am going to close this now. We can open new bugs if we find something interesting.