934500 - Don't eagerly create callsite clones in IonBuilder

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Using callsite clones is an optimization rather than a requirement, and if the clone has never been created at the point of Ion compilation then there isn't much reason to do it eagerly.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: updated

Oops, missed a couple CloneFunctionAtCallsite uses in PJS stuff.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 826792 [details] [diff] [review]
updated

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

Forwarding to shu; I'm not very familiar with callsite cloning.

::: js/src/jscntxt.cpp
@@ +132,3 @@
>          return nullptr;
>  
> +    Table::Ptr p = table.lookup(Key(fun, script, pc - script->code));

Won't this race with the main thread?

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 826792 [details] [diff] [review]
updated

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

The IonBuilder changes look pretty straightforward, but the race Jan found is a problem. See comments below.

::: js/src/jscntxt.cpp
@@ +132,3 @@
>          return nullptr;
>  
> +    Table::Ptr p = table.lookup(Key(fun, script, pc - script->code));

I agree with Jan here, it looks like it'll race. Replacing the lookup with readonlyThreadsafeLookup isn't enough, since the interpreter/baseline still mutates the callsite clone table.

A lock might be okay as contention should be pretty low between OMTC and the interpreter/baseline executing something that causes us to callsite clone.

That said, a longer term fix that would be nice is if we didn't clone the entire function into some compartment-wide table, but only cloned TypeScripts as we only care about the extra precision of observed types. Perhaps we can then keep the collection of TypeScripts for these clones somewhere else, such as on the TypeScript of the caller at the pc of the call. Since that collection will be pretty small in most cases, we can use a very simple data structure with atomic adds, like a linked list, and not worry about racing.

@@ +167,5 @@
> +    Key key(fun, script, pc - script->code);
> +    Table::AddPtr p = table.lookupForAdd(key);
> +    JS_ASSERT(!p);
> +
> +    if (!table.add(p, key, clone))

putNew, maybe?

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 826792 [details] [diff] [review]
updated

There is no race here yet, as IonBuilder still only runs on the main thread.  Before IonBuilder runs off thread I'll be adding a lock to protect changes to the callsite clone table.  With this change though the callsite clone table will only be changing on the main thread, so that (a) the main thread doesn't need to lock to read the table, (b) the main thread takes the lock when modifying the table, and (c) the compilation thread takes the lock when reading the table.

This is the synchronization strategy that will be used for pretty much all mutable state which can be accessed during compilation, and rather than adding locking piecemeal I want to take care of this later on once everything else is in place, e.g. here a problem is that IonBuilder has a bare JSCompartment pointer which it can do anything with; instead it should have a wrapper pointer which only exposes the interfaces the compiler needs and makes it easier to ensure that all the right locking is in place.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 826792 [details] [diff] [review]
updated

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

Looks fine to me. Still would prefer the putNew nit, and perhaps a comment to mark places in the source that would require locks once IonBuilder moves off-thread?

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d23caa617163

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d23caa617163