497207 - Define |enum JSTraceType| and stop overloading JSVAL_* for trace-time types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

We all knew what we had was wrong, but if it's troublesome enough to figure out what a trace-type number means that dmandelin chooses to turn around and ask me verbally rather than look it up (and worst of all that information's paged out of my memory), we really can't punt this any longer.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Change all trace types to JSTraceType enum values

Comment 2

[Brendan Eich [:brendan]]

I mentioned graydon filing a bug on this already, suggesting ASCII codes instead. I like that better when debugging, compared to casting uint8 to an enum type.

I was gonna dup this to that bug, bug 479507, but the two bugs want different solutions to the same problem. Can we agree quickly on which bug's prescribed fix is the right one? Ideally we'd have bugs state problems and not solutions, but that happens, understandably.

/be

Comment 3

[Brendan Eich [:brendan]]

Don't forget dmandelin -- peers all cc'ed, let's go. I say ASCII wins.

/be

Comment 4

[Andreas Gal :gal]

I prefer the 0..7 notation. It lets us pack type maps tighter if we get around to it. 8 choices are easy to remember. r=me otherwise

Comment 5

[Brendan Eich [:brendan]]

Could use enum with char codes as enumerator values, of course. Didn't mean to make the solutions disjoint.

Packing into 3 bits is not in front of us, but if it were, we could do that then. For now, which is more mnemonic: 'F' or 7?

/be

Comment 6

[Andreas Gal :gal]

1 is int, 2 is double, 3 is jsval, 4 is strong, 5 is null, 6 is object, 7 is function. And I didn't even peek. Come on. Everyone should have the jsval_tag alphabet down by now :)

Comment 7

[Andreas Gal :gal]

We can definitively change this later if we want to pack tighter. I am fine with whatever.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> 1 is int, 2 is double, 3 is jsval, 4 is strong, 5 is null, 6 is object, 7 is
> function. And I didn't even peek. Come on. Everyone should have the jsval_tag
> alphabet down by now :)

6 is boolean.

That aside...I don't think it's a problem if it's easy to look them all up in one place, and sharing with the jsval tagging cuts down the numbers to remember a ton.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #6)
> > 1 is int, 2 is double, 3 is jsval, 4 is strong, 5 is null, 6 is object, 7 is
> > function. And I didn't even peek. Come on. Everyone should have the jsval_tag
> > alphabet down by now :)
> 
> 6 is boolean.

The prosecution rests :-/.

/be

Comment 10

[David Mandelin [:dmandelin]]

I'm neutral between readable character or numeric codes. I just want to be able to look 'em up in one place, like Waldo says. Or see an enum name in the debugger.

Comment 11

[Graydon Hoare :graydon]

Typemaps need to be fully encapsulated before this sort of patch lands. There's a pile of logic that currently depends on them being uint8_t arrays.

Once beyond that, I only want three things:

  - I think the codes should *not* correspond to jsval tags; there should be very little ability to confuse the two. They're different type systems. Like really different. The range of a jsval-tagged int is actually different than the range of a JIT int (31 bit vs. 32). IMO the types should scream "I'm different" to a reader.

  - I'd prefer a form that a naked debugger will display properly without looking them up or having to x/foo some quantity of memory. I want to take every opportunity possible in coming months to remove the need to make complex incantations to gdb. The advantage of strings is that, by carrying 0-terminators, debuggers print them automatically in every local display mechanism, which they tend not to do for arrays of unknown length.

  - I'd also prefer an relatively large code space since I expect we'll be doing further specializations in the future, and think it quite false economy to be worrying about the size of a typemap on the entry and exit points of a trace. Trivial adjustments to codegen, IR layout, struct members, memory management policies or a dozen other knobs will all cost or gain more memory either way than this issue.

Comment 12

[Jeff Walden [:Waldo]]

I'm not convinced it's good for the two type classes to be that significantly different, particularly because it means more drudgery to convert between them in the cases where we do.  (No, I don't think that's a good thing!)  I'm also not convinced we need a large amount of space for specializations, mostly because we've needed so few expansions so far.

I tried implementing a class that would do lots of magic and force values to be correct, but you can't switch on a value that's an instance of a class, and there are issues of PODness when trying to make it work exactly in the desired manner.  What I think we really want are sized enums, which both gcc and MSVC do support, with some pain -- patch anon, fighting apparent heisenwarnings.

Comment 13

[Andreas Gal :gal]

>   - I'd also prefer an relatively large code space since I expect we'll be
> doing further specializations in the future, and think it quite false economy
> to be worrying about the size of a typemap on the entry and exit points of a
> trace. Trivial adjustments to codegen, IR layout, struct members, memory
> management policies or a dozen other knobs will all cost or gain more memory
> either way than this issue.

Did you measure this? In hotpath exit maps were 80% of the code cache. Our maps are a bit more compact. I would guess our typemaps are bout 50% of the code cache. Its certainly not irrelevant how densely we pack them. Anyone care to measure this?

Comment 14

[Jason Orendorff [:jorendorff]]

>+ * The types of values calculated during tracing, used to specialize operations
>+ * to the types of the constituent values.

"constituent" has several meanings that are plausible here, so it ends up being
unclear.  By "constituent values", do you mean "operands"?

Generally I think the comment could be better. I think it would be good to
mention typemaps and what they're for. Better here than at VMFragment, IMO.

>+                                            These loosely correspond to the
>+ * values of the JSVAL_* language types to share some brainprint, but we add a
>+ * few further divisions to enable further optimization at execution time. Do

The "to share some brainprint" justification is weak. Also post hoc, or at
least, that's not how I remember it. ;) Delete it?

>+ * not rely on this loose correspondence for correctness without adding static
>+ * assertions!

I agree, but right away we do this:

>+    return JSTraceType(JSVAL_TAG(v));

So we need some static assertions, right?

(I would prefer ASCII codes for the enums in the long run, if it doesn't cost perf.  It remains to be seen, I guess.)

You could go a bit further and change e.g. the type of the `type` argument to ValueToNative, or the return type of js_GetUpvarOnTrace (and other similar places). This bug or a separate one.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Force JSTraceType to a particular size when known to be possible

This changes a lot more and nearly eliminates uint8 from jstracer.cpp, and it should do much better on the debugger front.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: TT_BOOLEAN -> TT_PSEUDOBOOLEAN

Oops, forgot to use the renaming I'd though of in previous iterations but forgot in the one I just posted, should help with handling undefined correctly, hopefully.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 382612 [details] [diff] [review]
TT_BOOLEAN -> TT_PSEUDOBOOLEAN

Gah, missed the first review cancel, need to address that...

Comment 18

[Jeff Walden [:Waldo]]

Attachment: v2

Comment 19

[Jason Orendorff [:jorendorff]]

OK, just two things here...

1. Can TT_INTEGER be TT_INT32?  Just a thought.

2. Changing all the typemaps to JSTraceType* and using compiler-specific language extensions to make JSTraceType a byte seems lame to me.  Instead, how about a lightweight class for passing around typemaps, like this:

  class JSTypeMap {
  private:
      uint8_t *mPtr;
  public:
      JSTypeMap(uint8_t *p) : mPtr(p) {}

      /* default copy constructor and copy assignment operator are ok */

      JSTraceType operator[](size_t i) const {
	  return JSTraceType(mPtr[i]);
      }

      void put(size_t i, JSTraceType t) {
          mPtr[i] = t;
      }
      ...
  };

They should be as cheap as pointers to pass around, so getFullTypeMap et al
can return them.

Ideally they'd have range checking in DEBUG builds, but I don't know if the size of the typemap is always handy.

I'm sure this is not quite what Graydon meant by "typemaps need to be fully encapsulated" in comment 11, but I think just a little bit of encapsulation will mean we don't care if JSTokenType is 4 bytes.

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 383780 [details] [diff] [review]
v2

(Removing r? flag because I think this is the same version I reviewed.)

Comment 21

[Jeff Walden [:Waldo]]

...and massively bitrotted by FORALL removal as well -- it's taking a long time to update and resolve that concern.  :-\

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Updated past FORALL removal

First, I changed the JSTraceType values to be defined as an enum JSTraceType_ -- but with compilers that can't guarantee a size for the enum, I make a JSTraceType typedef that restricts it to one byte, otherwise it's a typedef directly to the enum.  I think this addresses most of the residual worry about typemap bloating for now, and we still get symbolic names when printing in a debugger (at least for all the compilers we care about).

TT_INT32 is a good call.

I'm leery of abstracting type maps even further that way given that we *already* have a TypeMap class, and having both TypeMap and JSTypeMap is just too much.  Further, and more importantly, I'd rather not take on too much at once in one bug; this thing bitrots really easily.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 385008 [details] [diff] [review]
Updated past FORALL removal

Could you refresh it one more time? I'd like to apply it and test a few things, and it's stale again. (sorry, I'm slow)

Comment 24

[Jeff Walden [:Waldo]]

Attachment: I love the experience of merging in the morning

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 385192 [details] [diff] [review]
I love the experience of merging in the morning

OK.

Comment 26

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/cc69258f3980

Comment 27

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/8d64e8df77b6 as bustage fix for (correctly?) quirky Windows compilers

Comment 28

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cc69258f3980