Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile non-inline non-module scripts off thread #26571

Closed
jdm opened this issue May 19, 2020 · 7 comments
Closed

Compile non-inline non-module scripts off thread #26571

jdm opened this issue May 19, 2020 · 7 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented May 19, 2020

https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/dom/script/ScriptLoader.cpp#2191-2205
https://searchfox.org/mozilla-central/rev/61fceb7c0729773f544a9656f474e36cd636e5ea/dom/script/ScriptLoader.cpp#2035-2069

Gecko has a model where inline scripts get evaluated directly, but external scripts get compiled off the main thread, then the parser resumes execution of the compiled bytecode once compilation is finished. This allows the page to continue responding to user input when a large script has just been loaded.

@gterzian
Copy link
Member

@gterzian gterzian commented May 23, 2020

assigned to @AbhishekSharma102

I will try to write up more details...

@gterzian
Copy link
Member

@gterzian gterzian commented May 23, 2020

So this would have to use js::jsapi::CompileOffThread, and pass a OffThreadCompileCallback that would be called by the JS engine with a token, and whatever we pass as void* callbackData.

Since the callback is called off the main-thread, we would need to then queue a task back on the event-loop/main-thread where the original call to CompileOffThread happened. I would say we should use the NetworkingTaskSource, available at

pub fn networking_task_source(&self) -> NetworkingTaskSource {

So I'd say our void* callbackData should be a struct similar to

struct TimerListener {
, that we would cast to void* callbackData when calling CompileOffThread.

So essentially this struct should own a Trusted<GlobalScope, a task-source and canceller, and other data that needs to be saved somewhere in between the initial call to CompileOffThread, and the actual handling of the compiled script back on the main thread.

With regards to the OffThreadCompileCallback, I'm not completely sure but I'm wondering if we could simply pass a unsafe extern "C" fn?

So when the actual OffThreadCompileCallback is called off the main thread, we should cast the void* callbackData back to our struct, and then use this struct to queue a task back on the main-thread, this task should use the token passed to the callback to get the compiled script by calling js::jsapi::FinishOffThreadScript, and it should then process the returned JsScript further.

For an example of task-queuing, see

let _ = self.task_source.queue_with_canceller(

With regards to where we should call CompileOffThread, one place might be over at

pub fn evaluate_script_on_global_with_result(
, although this is used for all script so you'd have to ensure the off-thread compilation option is only used when this is called into by
pub fn run_a_classic_script(&self, script: &ScriptOrigin) {

So essentially I think we'd have to break-up evaluate_script_on_global_with_result to the point up to when CompileOffThread is called, and then continue via the callback/task-queuing mechanism.

It seems to me that since we'll be operating on a JSScript, we could use js::jsapi::JS_ExecuteScript on it, instead of Evaluate2 like is currently used on the string.

@jdm do you have more to add or to correct?

@jdm
Copy link
Member Author

@jdm jdm commented May 23, 2020

That all sounds reasonable to me.

@gterzian
Copy link
Member

@gterzian gterzian commented May 23, 2020

@jdm thanks for checking

@AbhishekSharma102 let me know if you have any questions, for examples of using the js::jsapi and casting things around you can see quite a lot of examples in this file: https://github.com/servo/servo/blob/master/components/script/dom/bindings/structuredclone.rs

@AbhishekSharma102
Copy link
Contributor

@AbhishekSharma102 AbhishekSharma102 commented May 23, 2020

Thanks for all the information! I will take a look and comment in case of any doubts. All of these seem new, so might take a few days.

@AbhishekSharma102
Copy link
Contributor

@AbhishekSharma102 AbhishekSharma102 commented May 26, 2020

Hey! I had a couple of questions.

  1. In evaluate_script_on_global_with_result, we use CompileOptionsWrapper to cast the parameter to ReadOnlyCompileOptions needed by Evaluate2 which is provided by rust library. In JS_ExecuteScript, we need HandleObjectVector as a parameter, no such cast is available. Should i create it?
  2. I noticed that rust does not have optional parameters. As a result I will need to add a parameter to evaluate_script_on_global_with_result to ensure OffThreadCompilation is done only when called by run_a_classic_script. Is that fine or is there a better way?

To summarise my understanding, modify the evaluate_script_on_global_with_result for OffThreadEvaluation only when called by run_a_classic_script. For the off thread compilation, use the CompileOffThread api, and when done, queue the callbackData received by FinishOffThreadScript onto the main thread using networking_task_source after casting it into the new struct.

Please correct if wrong.

@gterzian
Copy link
Member

@gterzian gterzian commented May 26, 2020

  1. I think we can initially try to use the version of JS_ExecuteScript that takes only two parameters, and does not require an envChain.
  2. Yes, I would say the caller of evaluate_script_on_global_with_result can be represented by an enum, and then you can branch via a match on it.

Yes that appears correct, so you have to call CompileOffThread with the callback and callbackData params, with the first one can be I hope just an extern c function, and the second is the struct(cast to a pointer)that you'll use later, from the callback, to queue a task back on the main thread. So in the callback you have to cast void* callbackData back to the struct, and then pass the token to it and have it queue a task to "continue" evaluate_script_on_global_with_result(but a new branch of it using JS_ExecuteScript as opposed to Evaluate2).

You can just open the PR quite early and ask further questions there if you want.

bors-servo added a commit that referenced this issue Jul 9, 2020
added callback function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 10, 2020
added callback function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 11, 2020
added callback function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 20, 2020
Compile external scripts off the main thread

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 20, 2020
Compile external scripts off the main thread

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 21, 2020
Compile external scripts off the main thread

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 21, 2020
Compile external scripts off the main thread

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26571  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.