479888 - TM: kill builtins.tbl

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: kill builtins.tbl

The declaration of built-in functions and their corresponding CallInfo
struct is something of a mess.  The builtins.tbl file has macros defining
about half of them (those defined in jsbuiltins.cpp), but the ones in
jsarray.cpp, jsstr.cpp, etc, aren't defined in builtins.tbl.  

Here's how the current macros work, for js_EqualStrings (one that is not
named in builtins.tbl:

  // In jsbuiltins.h;  macro-declares the CallInfo struct, making it public.
  JS_DECLARE_CALLINFO(js_EqualStrings)

  // In jsstr.h;  hand-declares the function.
  extern JSBool JS_FASTCALL
  js_EqualStrings(JSString *str1, JSString *str2);    // declares the func

  // In jsstr.cpp;  macro-declares the function, macro-defines the Callinfo
  // struct, ensuring that they match.
  JS_DEFINE_CALLINFO_2(extern, BOOL,   js_EqualStrings, STRING, STRING, 1, 1) 

  // In jsstr.cpp: hand-declares/defines the function.
  JSBool JS_FASTCALL
  js_EqualStrings(JSString *str1, JSString *str2)
  {
      ...
  }

(It's a lot of repeated typing, but I can't see a good way to make it more
concise.)

The ones in builtins.tbl are similar, except the JS_DECLARE_CALLINFO and
JS_DEFINE_CALLINFO_<n> macros are generated from the builtins.tbl.  Note
that it's the JS_DEFINE_CALLINFO macro that's important for tying the
function type to the CallInfo struct type, *not* builtins.tbl.  So
builtins.tbl is just obfuscation, really.

The attached patch kills builtins.tbl;  basically it just inlines it in the two
places where it is currently #included.  The duplication isn't bad because
the JS_DECLARE_CALLINFO macros only take one argument, the function name.
Half the builtins are already done this way, so this just makes things more
consistent.  It also corrects a couple of minor inaccuracies in the
comments.  It also moves a couple of CallInfo methods from Assembler.cpp
to LIR.cpp, a better spot for them.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 363784 [details] [diff] [review]
kill builtins.tbl

Great. Thanks!

If you want to, please feel free to put each of those JS_DEFINE_CALLINFO macros right below the function it describes.  I think that wins in the long run.

r=me with or without that change.

You could go even further, moving functions from jsbuiltins.cpp into the files where they logically belong. Later, I guess.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: also move each JS_DEFINE_CALLINFO macro below its corresponding function

I put each macro call below its corresponding function, like you suggested.  One good thing about this is that previously some of the functions were within a "#ifdef JS_TRACER" but the corresponding macro call was not;  that's no longer the case.  I made the move for all of them, not just the ones in jsbuiltins.cpp.  While at it I removed the extraneous whitespace in the macro calls -- there's no point having it if they're not all together in a table-like form.

I didn't move the functions out of jsbuiltins.cpp -- for some of them it wasn't clear to me which file they would best be moved to.

Comment 3

[Nicholas Nethercote [inactive]]

ping?

Comment 4

[Andreas Gal :gal]

What about the 2nd patch? Does it need review?

Comment 5

[Nicholas Nethercote [inactive]]

I put jorendorff down again...

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 364023 [details] [diff] [review]
also move each JS_DEFINE_CALLINFO macro below its corresponding function

The nanojit change at the end of that patch isn't related to this stuff, right?  r=me with that part split out into a separate patch.

Comment 7

[Jason Orendorff [:jorendorff]]

I meant a separate changeset.  No need to re-review or file a separate bug (unless you actually intended that part to be in a separate bug...)

Comment 8

[Nicholas Nethercote [inactive]]

From the original description:

  It also moves a couple of CallInfo methods from Assembler.cpp
  to LIR.cpp, a better spot for them.

It's somewhat related in that the whole patch is cleaning up stuff related to CallInfos, which is why I included it.  I can take it out if you like, but I'm unclear about the distinction between a patch and a changeset... and if it gets separated doesn't it need a separate bug?

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: rebased patch that excludes the nanojit changes

I rebased the patch and removed the nanojit changes.  I don't have commit permissions -- can somebody (jorendorff?) land this for me?  Thanks.

Comment 10

[Andreas Gal :gal]

Did you verify that there is no perf impact?

Comment 11

[Andreas Gal :gal]

Actually the output might be binary-identical so we probably don't have to worry about #10. I will land it.

Comment 12

[Nicholas Nethercote [inactive]]

Also, the nanojit changes can be dropped from consideration for this bug --
they'll clash with the widen-LIR bug (#488775) and they're only minor.  I'll
deal with them later in another bug.

Comment 13

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/3c7c755e8b48

Comment 14

[Andreas Gal :gal]

no-jit builds were burning

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

Comment 15

[Nicholas Nethercote [inactive]]

Thanks for fixing it.  What testing should I have done to prevent that?

Comment 16

[Andreas Gal :gal]

configure --disable-jit and make with that

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3c7c755e8b48