575848 - JIT should early bind constructor calls to known builtins

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P3)

Description

[Edwin Smith]

Currently, early binding to constructor calls to Array, RegExp, Date, and other builtins are not early bound.  These classes have specialized construct sequences that don't follow the typical "newInstance + invoke constructor method" pattern.

Nevertheless, we are able to early bind, and common cases should be better optimized.   RegExp and Array have come up as prime candidates.

As discussed on bug 573504, early binding for RegExp could enable using an inline cache for compiled regular expression objects, instead of a global cache.

Comment 1

[Lars T Hansen]

I'm curious about the use of 'new Object' as well.  Speaking for myself I'd always use {} instead, but I doubt that's universal.

If we are able to early bind to new Array, could/should we rewrite the code to use the NEWARRAY bytecode?  ie, should "new Array(1,2,3,4)" be optimized into "[1,2,3,4]"?

Ditto, should "new Object" become a simple NEWOBJECT?

Comment 2

[Lars T Hansen]

See also bug #532690, which suggests that the JIT is either not very successful in binding to the Vector constructor, or that it does not exploit early binding to the Vector constructor well.

Comment 3

[Edwin Smith]

The early bound code for new Array() or new Object() could certianly be the same as for [] and {}.  the asc-generated sequence for the former would be something like:

  2         findpropstrict	Array
  4         pushbyte      	1
  6         pushbyte      	2
  8         constructprop 	Array (2)

vs

  15        pushbyte      	3
  17        pushbyte      	4
  19        newarray      	[2]

In the early binding case, findpropstrict would become dead; the pattern match would have to be something like:  findpropstrict (expr)* constructprop.  (expr could be nearly anything.  but once the jit sees constructprop, we can internally do what newarray does, and get the same effect as rewriting.

Then there's the problem that [N elements] chews up N stack slots; transforming into a series of stores could be a win, but will take other changes in the jit to actually acheive the stack space savings.  (the tags[] and vars[] arrays will have to be explicitly shortened somehow).  There's a few machine generated examples in Brightspot with N in the 100,000 range.  its init code, not likely to be worth jit-compiling anyway.  to win on that code, ASC must be changed.

Comment 4

[Edwin Smith]

I need to study the code paths a bit more, but now that early-bound construction is done via the VTable->createInstance pointer, it should be easy to support classes with custom createInstance code (Array, Object, etc).  Maybe just a matter of setting array_itraits->hasCustomConstruct = false, etc.

The win for {} and [] would be skipping the entire AS3 constructor invocation in favor of C++ code to initialize the array.  (e.g. Array::newarray).

Comment 5

[Edwin Smith]

Attachment: Set array_itraits->hasCustomConstruct = false since it uses an ordinary constructor

ArrayClass doesn't override ClassClosure::construct(), so its safe to set this false, and "new Array()" will use the standard vtable->createInstance(); /* call constructor */ sequence.

Cleaned up comments in emitConstruct().

Note that the failsafe case is used when we don't know the class type, or when we *do* know the class type and the class object overrides ClassClosure::construct().  In the latter case, this means the class has rolled newInstance and the instance constructor into one implementation in native code.  (RegExp does this).  

We call construct(argc,argv) directly from jit code; other than the cost of boxing arg values as Atom, this is still an early bound call -- no symbol table lookups occur, and the JIT still has full knowlege that the result object is the proper instance type.

Comment 6

[Lars T Hansen]

Comment on attachment 455128 [details] [diff] [review]
Set array_itraits->hasCustomConstruct = false since it uses an ordinary constructor

Since you capitalized one 'e' in the comment in CodegenLIR.cpp you might as well capitalize the other one too.

Comment 7

[Edwin Smith]

Comment on attachment 455128 [details] [diff] [review]
Set array_itraits->hasCustomConstruct = false since it uses an ordinary constructor

Comments further improved.  
TR: http://hg.mozilla.org/tamarin-redux/rev/3eb7970f07de

Comment 8

[Edwin Smith]

Currently, hasCustomConstruct defaults to true, as a conservative safeguard against builtin/native classes that ovverride CC::construct().  However, it appears we are way too conservative.

For builtins, lets assume we can explicitly opt-in correctly.  

For shell_toplevel, there are no classes that override CC:construct().  So, adding a loop to set all hasCustomConstruct = false, is easy.

for Flash/AIR, we have two options
 1. opt-in (current approach).  100's of classes need to be marked as early-bindable.
 2. opt-out (old / risky approach).  a handful of classes need to be marked as *not* early bindable.  but forgettin to do this causes crashes.

If we can build an assert mechanism to safeguard #2, we should do that.

Comment 9

[Edwin Smith]

Attachment: clear hasCustomConstruct on more classes

This patch clears hasCustomConstruct on even more classes, particularly
* builtin classes with no native subclass of ClassClosure
* builtin native classes that aren't in BuiltinTraits
* every class in shell_toplevel (none override ClassClosure::construct)

Clearing the flag on non-native builtins will take care of many many classes in Flash/AIR too.  The remaining ones are native and need more work to safely clear the flag.

Comment 10

[Edwin Smith]

Attachment: (v2) make hasCustomConstruct opt-in

This version inverts the default for hasCustomConstruct, so its false by default for *all* classes.  There's a dozen or so VM classes that must explicitly set this, and about two dozen in Flash.

To catch mistakes, we peek into the C++ vtable to check if the construct() function pointer is equal to ClassClosure::construct(), and this result must match hasCustomConstruct.   If not, compilain and assert.

The check is only enabled for GCC.  A second (longer) version of the check is included in the patch, that I beleive would work on any C++ compiler with a conventional vtable layout, but I haven't tested it.  As long as this assert is running for at least one mainstream compiler, that's enough.

Note that the previous default (true for all builtins) was more conservative than it needed to be: most builtins have no native class at all (ie aren't in the native class table generated by nativegen.py).  We could have been setting those guys to false automatically already.  If desired, I can do that in a separate patch.

This patch, however, goes all the way.  I submit that this is safe enough, with the assert to catch maintenance errors.

Comment 11

[Lars T Hansen]

Comment on attachment 455262 [details] [diff] [review]
(v2) make hasCustomConstruct opt-in

Neat.

Slap on the wrist for the ad-hoc (vi-imposed?) reformatting in NativeFunction.cpp.

Still scratching my head over your use of 'union obj' instead of casts in checkConstructOverride, but no serious objection.  (I see that pattern used throughout the code to deal with aliasing, but (a) I don't think it does that nearly as often as we think and (b) when all we're doing is reading memory aliasing shouldn't matter anyway.)

Comment 12

[Edwin Smith]

(In reply to comment #11)
> (From update of attachment 455262 [details] [diff] [review])
> Neat.
> 
> Slap on the wrist for the ad-hoc (vi-imposed?) reformatting in
> NativeFunction.cpp.

already reverted; a previous rev of the patch inserted code
there to mark all non-native builtins with hasCC = false.

> Still scratching my head over your use of 'union obj' instead of casts in
> checkConstructOverride, but no serious objection.  (I see that pattern used
> throughout the code to deal with aliasing, but (a) I don't think it does that
> nearly as often as we think and (b) when all we're doing is reading memory
> aliasing shouldn't matter anyway.)

I'll clean up.  I think union memptr is useful, but union obj? not so much.

any opinions about the longer version?  if it really were portable across our supported compilers, would we prefer it over the shorter gcc version?  I think the only argument in favor of a portable version is if we used it to compute hasCustomConstruct automatically (rather than as a safety check).

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> any opinions about the longer version?  if it really were portable across our
> supported compilers, would we prefer it over the shorter gcc version?  I think
> the only argument in favor of a portable version is if we used it to compute
> hasCustomConstruct automatically (rather than as a safety check).

The value of the longer version might be to catch builtins with overridden constructors that are platform-specific.  I doubt we have any such today, and some of the idea behind AIR is that there won't be any of those, but I would be surprised if that idea holds up 100% over time.  But I think the gains of worrying about that are slight.  Also, if there is a custom constructor then presumably it does something useful that a test case would test, and the failure to set hasCustomConstruct would be discovered.

I'm happy to stick with the GCC version myself, GCC is the closest thing we have to a reference compiler.

Comment 14

[Steven Johnson]

Comment on attachment 455262 [details] [diff] [review]
(v2) make hasCustomConstruct opt-in

GCC checking is probably adequate for our needs.

Nit: in the if-0'ed, code you have

    struct CC1: public ClassClosure

but this is illegal in some compilers (struct must extend struct)

Comment 15

[Steven Johnson]

Does this set of patches make https://bugzilla.mozilla.org/show_bug.cgi?id=530222 obsolete?

Comment 16

[Rick Reitmaier]

Would it make sense to add the construct check testing into the selftests infrastructure rather than ifdef DEBUG ?

Comment 17

[Edwin Smith]

A self tests would have to explicitly instantiate every concrete subclass of ClassClosure, to test whether the overriding is correct.  That might be possible when the native library is initialized, I think there's a table of native classes.  If we ran the check once during native code bootstrap, it would not require messing with build/test infrastructure.

The existing DEBUG code seemed easier, but you're right that if a particular native *class* is never instantiated, it won't catch any mistake.  I'm inclined to leave it as-is for the sake of simplicity and create a followon bug.  Any strong opinions?

Comment 18

[Edwin Smith]

(In reply to comment #15)
> Does this set of patches make
> https://bugzilla.mozilla.org/show_bug.cgi?id=530222 obsolete?

Yes, I think so.  We will want to refine this code in the player:

for (uint32_t i = 0; i < class_count; ++i)
{
    m_playerPool->getClassTraits(i)->itraits->hasCustomConstruct = true;
}

it will trigger the assert.  I audited the avmglue code and only found ~25 classes where the value needed to be true, so I think we should just delete the above loop and replace it with explicit settings for the known classes.

Comment 20

[Rick Reitmaier]

(In reply to comment #17)
>  I'm inclined
> to leave it as-is for the sake of simplicity and create a followon bug.  Any
> strong opinions?

Sounds reasonable to me.

Comment 21

[Edwin Smith]

As a rule, what can we say if a class sets hasCustomConstruct, and is overridden?

A subclass that did not also have a custom construct() method on its ClassClosure object would simply use ClassClosure::construct.  So, early binding to that subclass's constructor would correctly inline CC::construct(), which calls (customizeable) newInstance() and invokes the constructor.

Such a thing may or may not be valid, however early binding should not matter one way or the other.  Correct?

Comment 22

[Edwin Smith]

Attachment: (v3) make hasCustomConstruct opt-in

Rebased patch again.  old one is already R+, but looking for ideas on how to manage the larger and build-dependent set of player classes that need to set hasCustomConstruct = true.  

a) metadata in AS, processed by nativegen?  good for ongoing maintenance, but doesn't help in fleshing out the initial list of classes that need an override.

b) dehydra analyzer. temping, if i can get the player build going on linux.

c) evil.  in interp-only mode where its safe, skip the assert and print the classname.  run each target to get the list, then update source code.

Comment 23

[Edwin Smith]

Attachment: Analyzer that finds classes using a custom construct() method, which requires setting hasCustomConstruct=true

Comment 24

[Edwin Smith]

Given a list of C++ classnames in classes.log, this command produces the list of abcclass constants used for setting traits.hasCustomConstruct = true

  for class in `cat classes.log`; do
    grep $class builtin.cpp | grep abcclass
  done | sed -E -e 's/ *AVMTHUNK_NATIVE_CLASS\(//' -e 's/, .*//' | sort

for host classes, replace "builtin.cpp" with the output file from nativegen.py, e.g. shell_toplevel.cpp, avmglue.cpp, etc.

Comment 25

[Edwin Smith]

(In reply to comment #22)

> b) dehydra analyzer. temping, if i can get the player build going on linux.

After an afternoon of grep, compile, test, assert, repeat, approach (c) above was looking pretty bad.  So I decided to try to learn Dehydra and attempt (b).  So far so good.  The attached analyzer is able to find the interesting classes in avmshell, and it found three more classes in the player than I did by hand (which still needs testing to validate).

Todo:

* make sure script doesn't miss any classes due to early bailout while iterating up the class tree

* check all the various build targets (ifdef'd out classes could get missed)

Comment 26

[Edwin Smith]

Attachment: (v4) make hasCustomConstruct opt-in

Changes since last patch: rebased, and added a customconstruct attribute to [native()] metadata.

This puts the attribute on the class def that needs it, and puts the boolean in NativeInfo so we can set it in AbcParser without maintaining a shadow list of 'tainted' classes in C++ source code.  It also avoids the need to track down the abcclass mangled name constants.

This turns out to be important because AIR has many variant builds of the builtins, and maintaining those flags in parallel will be too fragile, even with the help of an assert.

Comment 27

[Steven Johnson]

Comment on attachment 469546 [details] [diff] [review]
(v4) make hasCustomConstruct opt-in

the check in CodegenLIR.cpp went from !itraits->hasCustomConstruct to !ctraits->hasCustomConstruct, is this correct?

Comment 28

[Edwin Smith]

intentional change, yes.  after all, its a class property, i had no idea why we were setting it on the instance traits instead of the class traits.  If you know of a good reason, tell me and I'll change it back.

Comment 29

[Steven Johnson]

Nope, no idea why it wasn't there in the first place.

Comment 30

[Edwin Smith]

Attachment: (v5) make hasCustomConstruct opt-in

Review ready patch.  Just rebased since v4.

Comment 31

[Edwin Smith]

Attachment: (v5) 468883: Analyzer that finds classes using a custom construct() method

This is the Dehydra script to list C++ classnames that override ClassClosure::construct().

It will print a classname if:
 * it extends avmplus::ClassClosure
 * it declares a construct() function that matches the signature of
   ClassClosure::construct
 * OR, one of its bases extends & overrides ClassClosure::construct

The logic does not depend on iteration order of base classes and thus should
work with multiple inheritance.

Classnames are printed to <filename>.customconstruct in whatever directory
contains .o files.  Then they were collated with this script:

find <build-dir> -name \*.customconstruct | xargs cat |sort -u >classes-<build>.log

I built in several configurations, then listed the AS3 class declarations that needed to be edited with this pipeline:

  for c in `cat classes*.log | sort -u`; do \
    find * -name \*.as -print0 | xargs -0 grep "cls=\"$c\""; \
  done

I then hand-edited each .as file to add a customconstruct=true attribute to the [native] metadata line.  (I suppose sed could have done this!)

Comment 32

[Edwin Smith]

TR:

Comment 33

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/b9acfacf267d

Comment 34

[Brent Baker]

(In reply to comment #33)
> TR: http://hg.mozilla.org/tamarin-redux/rev/b9acfacf267d

The follow-up change to fix the ARM breakage does not work for MIPS:
http://hg.mozilla.org/tamarin-redux/rev/a7d9f46939eb

Debug version of the MIPS shell asserts with the following:
Assertion failed: "((ctraits->hasCustomConstruct == overrides_construct))" ("/home/build/buildbot/tamarin-redux/linux-mips/repo/core/MethodEnv.cpp":791)

Comment 35

[Rick Reitmaier]

Brent is this still an(In reply to comment #34)
> (In reply to comment #33)
> > TR: http://hg.mozilla.org/tamarin-redux/rev/b9acfacf267d
> 
> The follow-up change to fix the ARM breakage does not work for MIPS:
> http://hg.mozilla.org/tamarin-redux/rev/a7d9f46939eb
> 
> Debug version of the MIPS shell asserts with the following:
> Assertion failed: "((ctraits->hasCustomConstruct == overrides_construct))"
> ("/home/build/buildbot/tamarin-redux/linux-mips/repo/core/MethodEnv.cpp":791)

Brent is this still an issue?  If so, please open a new bug referring to this change.

Comment 36

[Brent Baker]

(In reply to comment #35)
> Brent is this still an(In reply to comment #34)
> > (In reply to comment #33)
> > > TR: http://hg.mozilla.org/tamarin-redux/rev/b9acfacf267d
> > 
> > The follow-up change to fix the ARM breakage does not work for MIPS:
> > http://hg.mozilla.org/tamarin-redux/rev/a7d9f46939eb
> > 
> > Debug version of the MIPS shell asserts with the following:
> > Assertion failed: "((ctraits->hasCustomConstruct == overrides_construct))"
> > ("/home/build/buildbot/tamarin-redux/linux-mips/repo/core/MethodEnv.cpp":791)
> 
> Brent is this still an issue?  If so, please open a new bug referring to this
> change.

Edwin addressed the issue in this changeset:
http://hg.mozilla.org/tamarin-redux/rev/cc4d77ea226d