1432794 - Investigate improving off-thread parsing performance by skipping object prototype initialisation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

When we parse a script off-thread, we create a new global for it and initialise the prototypes and constructors of any objects that might get created by the parser.  This happens on the main thread and for a small script can take a significant amount of time.  This effort is wasted because we don't use these objects in execution: parser created objects have their prototypes replaced with the target global's versions when the results of the parse are merged back to the main thread.

This bug is to investigate improving this situation by creating 'fake' prototype objects (and skipping the usual initialisation) and doing it off-thread.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1432794-skip-proto-init

This patch adds a path where resolving a constructor for a global is allowed to happen off-thread and rather than doing the usual initialisation creates a fake prototype / constructor.  The prototype is a placeholder object that records the global's slot that contains it.  This is used when merging compartments to find the correct object to replace it.  The constructor is set to a magic value.

A similar thing happens for creating other prototype objects via GlobalObject::getOrCreateObject() which is used for creating module related prototypes.

This allows prototype creation to happen on-demand and off-thread.  It simplifies creating off thread parse tasks and it simplifies the task of merging the compartments after parsing.

One issue is that when merging I had to call markUnknown() on object groups after replacing their prototype (otherwise I got assertion failures running Ion compiled code).  I'm still not sure if this is really a safe thing to do.  And maybe this ends up precluding some optimisations?

Here are some benchmark results:

Without patch:

    Serial compile empty script:   0.295 0.29 0.318 mS
    Parallel compile empty script: 0.217 0.31 0.24 mS
    Serial compile large script:   2.339 2.308 2.335 mS
    Parallel compile large script: 0.886 0.865 0.855 mS

With patch:

    Serial compile empty script:   0.084 0.092 0.102 mS
    Parallel compile empty script: 0.051 0.05 0.06 mS
    Serial compile large script:   2.226 2.244 2.231 mS
    Parallel compile large script: 0.734 0.725 0.723 mS

This is for compiling a script 1000 times, either serially or in parallel batches of 8.  The large script is a 50K expression that folds to a single constant and so takes negligible time to execute.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8945098 [details] [diff] [review]
bug1432794-skip-proto-init

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

Great idea! Nice speedup too.

::: js/public/Value.h
@@ +216,5 @@
>      /** uninitialized lexical bindings that produce ReferenceError on touch. */
>      JS_UNINITIALIZED_LEXICAL,
>  
> +    /** standard constructors are not created for off-thread parsing. */
> +    JS_OFF_THEAD_CONSTRUCTOR,

Nit: THREAD, typo

::: js/src/jsgc.cpp
@@ +7939,5 @@
> +            JSObject* obj = proto.toObject();
> +            if (GlobalObject::isOffThreadPrototypePlaceholder(obj)) {
> +                JSObject* targetProto = global->getPrototypeForOffThreadPlaceholder(obj);
> +                group->setProtoUnchecked(TaggedProto(targetProto));
> +                group->markUnknown(cx);

Groups for certain protos should have unknown properties, maybe that triggered the assertion failures. I think we should:

(1) MOZ_ASSERT(targetProto->isDelegate()); -- If that fails we need to call setDelegate (all proto objects need to have the delegate flag set).

(2) Call markUnknown if targetProto->isNewGroupUnknown().

I hope that helps.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1432794-skip-proto-init v2

(In reply to Jan de Mooij [:jandem] from comment #2)
> Nit: THREAD, typo

Duh, fixed.

> Groups for certain protos should have unknown properties, maybe that
> triggered the assertion failures. I think we should:
> 
> (1) MOZ_ASSERT(targetProto->isDelegate()); -- If that fails we need to call
> setDelegate (all proto objects need to have the delegate flag set).
> 
> (2) Call markUnknown if targetProto->isNewGroupUnknown().
> 
> I hope that helps.

Great, thanks.  That works.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8945732 [details] [diff] [review]
bug1432794-skip-proto-init v2

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

Looks good

::: js/src/vm/GlobalObject.h
@@ +853,5 @@
> +    {
> +        static const int32_t SlotIndexSlot = 0;
> +        static const Class class_;
> +        static OffThreadPlaceholderObject* New(JSContext* cx, unsigned slot);
> +        inline int32_t getSlotIndex() const;

Nit: can remove the inline keyword here and in the cpp file

::: js/src/vm/HelperThreads.cpp
@@ +800,5 @@
>      if (!global)
>          return false;
>  
> +    // Mark the global's zone group as created for a helper thread. This
> +    // prevents it from being collected until clearUsedByHelperThread() is

Pre-existing but there's also an AutoSuppressGC in this function - it's not sufficient to prevent collection?

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Pre-existing but there's also an AutoSuppressGC in this function - it's not
> sufficient to prevent collection?

That comment could do with expanding.  The 'created for helper thread' state persists until after parsing is complete if the function returns successfully.  AutoSetCreatedForHelperThread will clear the state automatically if the function fails, but otherwise the |createdForHelper.forget()| stops the state being cleared until much later.  We don't collect these zones at all until parsing is finished.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4d5be72031
Skip prototype and constructor intialization for off-thread parsing r=jandem

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1b4d5be72031

Comment 8

[Ionuț Goldan [:igoldan]]

This brought in some noticeable performance improvements!

== Change summary for alert #11344 (as of Tue, 30 Jan 2018 16:01:54 GMT) ==

Improvements:

  7%  tp6_facebook_heavy linux64 opt e10s     180.38 -> 168.12
  5%  tp6_facebook linux64 opt 1_thread e10s  180.08 -> 171.88
  4%  tp6_facebook linux64 opt e10s           179.90 -> 171.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11344