1456774 - Removing a parse task does a linear search with the helper thread lock held

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jon Coppeard (:jonco)]

When we finish an off-thread parse task we do a linear search through a vector of finished tasks.  This makes off-thread parsing quadratic in the number of scripts.  

This is probably not a big problem, but it could be an issue in pathalogical cases and it showed up while benchmarking off-thread parsing.  Also the script loader doesn't finish parse tasks until it comes to run them so it could potentially let a number of finished tasks build up.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1456774-finish-parse-task

This patch changes the finished parse task list to be a linked list rather than a vector so that we can remove elements in constant time.

I also changed the parse token to have its own type rather than using void* for this, and pushed that change through the browser.  I'm unsure why we didn't just trust the embedding to get this right, but maybe its because it involves passing the token between threads.  If you think this is still risky I can add a magic value or something.

Requesting review for js/src parts of this only.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8970905 [details] [diff] [review]
bug1456774-finish-parse-task

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

Good find. I like the improved type safety.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8970905 [details] [diff] [review]
bug1456774-finish-parse-task

Requesting review for the gecko parts of this.  This is just giving the void* parse token its own type.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8970905 [details] [diff] [review]
bug1456774-finish-parse-task

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

The DOM part looks OK, it's basically a renaming void* to JS::OffThreadToken*. Would be nice to have a comment explaining how this token is kept alive.

::: dom/base/nsJSUtils.cpp
@@ +196,5 @@
>    }
>  }
>  
>  nsresult
> +nsJSUtils::ExecutionContext::JoinAndExec(JS::OffThreadToken **aOffThreadToken,

** next to the type: JS::OffThreadToken** aOffThreadToken

@@ +323,5 @@
>    return mRv;
>  }
>  
>  nsresult
> +nsJSUtils::ExecutionContext::DecodeJoinAndExec(JS::OffThreadToken **aOffThreadToken)

same here.

::: dom/base/nsJSUtils.h
@@ +157,5 @@
>      // this function will take the result of the parser by moving it to the main
>      // thread before starting the execution of the script.
>      //
>      // The compiled script would be returned in the |aScript| out-param.
> +    MOZ_MUST_USE nsresult JoinAndExec(JS::OffThreadToken **aOffThreadToken,

here too

@@ +177,5 @@
>  
>      // After getting a notification that an off-thread decoding terminated, this
>      // function will get the result of the decoder by moving it to the main
>      // thread before starting the execution of the script.
> +    MOZ_MUST_USE nsresult DecodeJoinAndExec(JS::OffThreadToken **aOffThreadToken);

and here.

::: dom/xul/nsXULElement.cpp
@@ +2614,5 @@
>      return receiver->OnScriptCompileComplete(script, script ? NS_OK : NS_ERROR_FAILURE);
>  }
>  
>  static void
> +OffThreadScriptReceiverCallback(JS::OffThreadToken* aToken, void *aCallbackData)

can you change: |void *| to |void* |

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df68fd0a3eb1
Remove linear search for finished parse task and type off thread parse token r=jandem r=baku

Comment 6

[Dorel Luca [:dluca]]

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