Remove clone-at-call-site functionality

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: luke)

Tracking

unspecified
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
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.
(Assignee)

Comment 2

3 years ago
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.
Ok, I'm sold :)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
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.
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.
(Assignee)

Comment 7

3 years ago
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

3 years ago
(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.
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...
(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.
(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.
(Assignee)

Comment 12

3 years ago
And you're not worried about callsite cloning cloning the self-hosted script at every callsite?
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.
(Assignee)

Comment 14

3 years ago
That sounds like a pretty weak argument for keeping this around.
(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

3 years ago
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

3 years ago
s/TypedObject/TypedArray

In the TypedObject case the element types aren't actually finite and enumerable.
(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.
(Reporter)

Comment 19

3 years ago
I'm disentangling this bug from the PJS removal work.
No longer blocks: 1117724
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.
(Assignee)

Comment 21

3 years ago
Created attachment 8560546 [details] [diff] [review]
rm-callsite-cloning

Wow, that was a lot more invasive than I had guessed.
 33 files changed, 136 insertions(+), 750 deletions(-)
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8560546 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Attachment #8560546 - Flags: review?(shu)
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.
Attachment #8560546 - Flags: review?(jdemooij) → review+

Comment 23

3 years ago
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/
Attachment #8560546 - Flags: review?(shu) → review+
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/40c95e7f4f4b
https://hg.mozilla.org/mozilla-central/rev/40c95e7f4f4b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.