Closed Bug 924611 Opened 6 years ago Closed 6 years ago

Don't create lazy type objects and type properties in IonBuilder

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
To allow the main thread to read type information lock free, if IonBuilder is running off thread it can't change such information when lazily instantiating object types or properties.  The attached patch changes IonBuilder to only rely on type information that already exists when it first runs.  The main problem this presents is that type properties for singleton objects which are used during the compilation may not have been instantiated yet; such properties are treated by IonBuilder as if they are empty, which can cause compilation to fail and restart when finishing it.  To avoid this, baseline tries to more eagerly instantiate the properties when attaching caches.  As with bug 922270, this can increase memory usage for scripts which are baseline compiled but not ion compiled, but as with bug 922270 the extra memory should be small compared to the memory used by the baseline scripts/caches themselves.
Attachment #814549 - Flags: review?(jdemooij)
Component: JavaScript Engine → JavaScript Engine: JIT
Blocks: 925146
Comment on attachment 814549 [details] [diff] [review]
patch

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

::: js/src/jit/BaselineIC.cpp
@@ +5322,5 @@
>  
>      RootedId id(cx, NameToId(name));
>  
> +    // Instantiate this global property, for use during Ion compilation.
> +    types::EnsureTrackPropertyTypes(cx, global, NameToId(name));

Can we do this only if IsIonEnabled(cx), here and below?

::: js/src/jit/CodeGenerator.cpp
@@ -2151,5 @@
>      // If the function is known to be uncompilable, only emit the call to InvokeFunction.
>      ExecutionMode executionMode = gen->info().executionMode();
>      if (apply->hasSingleTarget()) {
>          JSFunction *target = apply->getSingleTarget();
> -        if (!CanIonCompile(target, executionMode)) {

What's the danger here exactly? Racing with script relazifying?

::: js/src/jsinfer.cpp
@@ +727,5 @@
> +    {
> +        JS_ASSERT(CurrentThreadCanAccessRuntime(jit::GetIonContext()->runtime));
> +        JSObject *singleton = isSingleObject() ? asSingleObject() : asTypeObject()->singleton;
> +        if (singleton && singleton->isNative() && singleton->nativeLookupPure(id)) {
> +            if () {

Syntax error.

@@ +728,5 @@
> +        JS_ASSERT(CurrentThreadCanAccessRuntime(jit::GetIonContext()->runtime));
> +        JSObject *singleton = isSingleObject() ? asSingleObject() : asTypeObject()->singleton;
> +        if (singleton && singleton->isNative() && singleton->nativeLookupPure(id)) {
> +            if () {
> +                EnsureTrackPropertyTypes(jit::GetIonContext()->cx, singleton, id);

Can we pass a JSContext *maybecx to this function? GetIonContext() can be really slow and I've removed calls to it that showed up in profiles...
Attachment #814549 - Flags: review?(jdemooij)
Attached patch updatedSplinter Review
Updated per comments.  The CanIonCompile() check was removed because it is racing with main thread changes to script->ion (even pre 785905) and checking for uncompilable scripts isn't a worthy optimization anymore since jitcode can directly call either baseline or ion scripts now.
Assignee: nobody → bhackett1024
Attachment #814549 - Attachment is obsolete: true
Attachment #815985 - Flags: review?(jdemooij)
Attachment #815985 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/f613d7363bd2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 927195
Depends on: 926847
You need to log in before you can comment on or make changes to this bug.