1121433 - Remove clone-at-call-site functionality

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

This functionality appears only to have been used by PJS, though I'm not 100% sure about that yet (could be called from Firefox?  Used by scripts external to the engine's selfhosted?).  We should probably remove it if it is unused, though.

Comment 1

[Till Schneidereit [:till]]

I'd actually prefer we didn't remove it. Ideally someone with some time on their hands would experiment with enabling it for, say some of the Array methods and see how performance is in circumstances as described by shu in http://rfrn.org/~shu/2013/03/20/two-reasons-functional-style-is-slow-in-spidermonkey.html.

Comment 2

[Luke Wagner [:luke]]

Callsite cloning adds non-trivial engine complexity of a central kind (it affects the basic VM representation; it gets tested in JSOP_CALL), so I think we should think twice before keeping it around.  Also, iirc, and perhaps this changed since I last payed attention, it's a rather brutish solution: we a priori mark specific functions as callsite-cloned and when we call them, we don't just create a cloned function object, we clone _the entire JSScript_.  As a hack to enable very specific PJS use cases, I can understand this, but as a general way of optimizing Array functions, this seems like the wrong strategy and likely to lead to performance regressions unless we further nuance the feature.  In general, as shu has said many times, callsite-cloning was a hack around the more general need for context-sensitive TI information and I think, if we want to better optimize the Array methods, we should go this direction since context-sensitive TI would also help user-written higher-order code.

Comment 3

[Till Schneidereit [:till]]

Ok, I'm sold :)

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #2)
> Callsite cloning adds non-trivial engine complexity of a central kind (it
> affects the basic VM representation; it gets tested in JSOP_CALL), so I
> think we should think twice before keeping it around.  Also, iirc, and
> perhaps this changed since I last payed attention, it's a rather brutish
> solution: we a priori mark specific functions as callsite-cloned and when we
> call them, we don't just create a cloned function object, we clone _the
> entire JSScript_.  As a hack to enable very specific PJS use cases, I can
> understand this, but as a general way of optimizing Array functions, this
> seems like the wrong strategy and likely to lead to performance regressions
> unless we further nuance the feature.  In general, as shu has said many
> times, callsite-cloning was a hack around the more general need for
> context-sensitive TI information and I think, if we want to better optimize
> the Array methods, we should go this direction since context-sensitive TI
> would also help user-written higher-order code.

I would love to see better context sensitivity in TI. The complexity required for that though, I believe, will dwarf whatever complexity, however uncouth, callsite cloning introduced. There're 2 concrete things I would like see done: 1) parameterize TypeScript (the thing that keeps recorded types) somehow on some notion of context, 2) do type refinement during inlining in Ion.

I want to note that when I wrote callsite cloning, it was after I experimented with parameterizing function TypeScripts on 'this' and argument types. I failed to get it to work after a few tries because it was *more* invasive to the VM than callsite cloning.

Comment 5

[Luke Wagner [:luke]]

I believe it would be more work, but it could be justified as it would help real user code.

On this tangent, though, I wonder if, instead of trying to solve the general context-sensitivity problem in TI, consider only the cases where Ion _can_ inline all the way through.  In these cases we are currently limited by using the original callee script's TypeScript, which is context-insensitive.  But what if, when inlining, we synthesized new TypeScripts that specifically represented Ion-inlined functions?  I'm not really familiar enough with the details of TI to know if this actually makes sense, but it seems like it could be an easier problem.

Comment 6

[Jeff Walden [:Waldo]]

I was planning on using this for the various typed array methods that are currently C++ that are going to be self-hosted, for what it's worth.  Not clear to me how many of them would be inlined (given that they will often contain not-entirely-trivial loops), and element-type-specific performance won, without this.  But full investigation still needs doing.

Comment 7

[Luke Wagner [:luke]]

Investigation soon or some...time.  It'd be nice not to keep this feature around in limbo for an unbounded amount of time.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #5)
> I believe it would be more work, but it could be justified as it would help
> real user code.
> 
> On this tangent, though, I wonder if, instead of trying to solve the general
> context-sensitivity problem in TI, consider only the cases where Ion _can_
> inline all the way through.  In these cases we are currently limited by
> using the original callee script's TypeScript, which is context-insensitive.
> But what if, when inlining, we synthesized new TypeScripts that specifically
> represented Ion-inlined functions?  I'm not really familiar enough with the
> details of TI to know if this actually makes sense, but it seems like it
> could be an easier problem.

Yeah, that's more or less what I called "type refinement during inlining" in comment 4. I'm not sure we even need to synthesize a new TypeScript -- we can probably intersect types directly when making new TemporaryTypeSets or something.

Comment 9

[Jan de Mooij [:jandem]]

FWIW, I'm looking at a polymorphic inlining bug atm (bug 900849) and callsite cloning adds complexity there too. Shu, do you agree we should remove this? If we're going to remove it now I should probably hold off on bug 900849 for now to avoid merge conflicts...

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> If we're going to remove it now I should probably hold off on
> bug 900849 for now to avoid merge conflicts...

Nevermind, bug 900849 doesn't require very invasive changes so it's not blocked on this one.

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #7)
> Investigation soon or some...time.

Soon.  Specifically, I want us to not have any typed array methods implemented in C++ by the end of the quarter.

Comment 12

[Luke Wagner [:luke]]

And you're not worried about callsite cloning cloning the self-hosted script at every callsite?

Comment 13

[Jeff Walden [:Waldo]]

I'm not "not worried" about anything about it, it was an idea to try and evaluate, then  once I had actual data on it I'd determine whether it was the most effective means or not.  It's possible a better idea instead is to switch on element type and have N different self-hosted methods to delegate behavior to, mimicking the way we do it with templated C++ methods now, just without the convenience of definition of templates.  As I said, I haven't investigated it yet.

Comment 14

[Luke Wagner [:luke]]

That sounds like a pretty weak argument for keeping this around.

Comment 15

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> It's possible a better idea instead is to
> switch on element type and have N different self-hosted methods to delegate
> behavior to, mimicking the way we do it with templated C++ methods now, just
> without the convenience of definition of templates.  As I said, I haven't
> investigated it yet.

Ah, so what we could probably do fairly easily is introduce `_cloneFunction` intrinsic that we run during self-hosting initialization to clone these functions for each of the TypedArray ctors. The implementation is identical, after all.

Comment 16

[Shu-yu Guo [:shu]]

If the contexts according to which a function should be cloned is finite and enumerable, it would be preferable to manually clone them anyways. In the TypedObject case, it sounds like the contexts are the element types, which are finite and enumerable.

Callsite cloning was a hack to approximate a posteriori context sensitivity. If the function doesn't have that kind of context sensitivity, then it shouldn't be used.

Comment 17

[Shu-yu Guo [:shu]]

s/TypedObject/TypedArray

In the TypedObject case the element types aren't actually finite and enumerable.

Comment 18

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> s/TypedObject/TypedArray
> 
> In the TypedObject case the element types aren't actually finite and
> enumerable.

True, though we could theoretically create a clone for each type the function is called on:

// Pseudocode:
var typedObjectTypesToMapFnMap = new Map();
function TypedObjectArrayMap(...) {
  var receiverType = this.constructor;
  var specializedMap = typedObjectTypesToMapFnMap.get(receiverType);
  if (!specializedMap) {
    specializedMap = _cloneFunction(TypedObjectArrayMapImpl);
    typedObjectTypesToMapFnMap.set(receiverType, specializedMap);
  }
  return specializedMap(...);
}

If _cloneFunction creates relazifiable functions, this wouldn't even be much of a memory usage issue. I guess another question is how the overhead for setting up the clone would compare with the runtime improvement. Potentially, we could only do this for large lists or something like that.

Comment 19

[Lars T Hansen [:lth]]

I'm disentangling this bug from the PJS removal work.

Comment 20

[Jan de Mooij [:jandem]]

Cloning self-hosted scripts at each callsite will likely be too heavy-weight for typed array functions. If there are serious performance issues with our self-hosted code, we can come up with other ways to fix them and hopefully that will benefit all JS.

So I agree with Luke, if there's no compelling reason to keep this let's remove it. It'd simplify some code that's complicated enough without this (polymorphic inlining for instance).

I'm also pretty sure that if we wait much longer, the code will be broken anyway.

Comment 21

[Luke Wagner [:luke]]

Attachment: rm-callsite-cloning

Wow, that was a lot more invasive than I had guessed.
 33 files changed, 136 insertions(+), 750 deletions(-)

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8560546 [details] [diff] [review]
rm-callsite-cloning

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

Nice! Great to see this simplifies the IonBuilder inlining code quite a bit.

::: js/src/jit/MIR.h
@@ +9253,5 @@
>      bool hasFunction(JSFunction *func) const;
>      types::TemporaryTypeSet *buildTypeSetForFunction(JSFunction *func) const;
>  
>      // Remove targets that vetoed inlining from the InlinePropertyTable.
> +    void trimTo(const ObjectVector &targets, BoolVector &choiceSet);

Thanks for making this const. While you're here, can you do the same for choiceSet? It makes the code a lot more readable.

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 8560546 [details] [diff] [review]
rm-callsite-cloning

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

Looks good!

::: js/src/jsscript.h
@@ +831,5 @@
>      // source object. Instead, the clone refers to a wrapper.)
>      js::HeapPtrObject sourceObject_;
>  
>      js::HeapPtrFunction function_;
> +    js::HeapPtrObject   enclosingStaticScope_;

\o/

Comment 24

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40c95e7f4f4b

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40c95e7f4f4b