Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCompile external scripts off the main thread #26710
Conversation
highfive
commented
May 29, 2020
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @ferjm (or someone else) soon. |
highfive
commented
May 29, 2020
|
Heads up! This PR modifies the following files:
|
highfive
commented
May 29, 2020
|
Hey @gterzian I have some questions regarding this.
Sorry if some questions seem trivial. |
| callbackData: *mut c_void, | ||
| ) { | ||
| let castedCallbackData = callbackData as *mut OffThreadCompilationData; | ||
| let script: JSScript* = FinishOffThreadScript(*(castedCallbackData.context), token); |
This comment has been minimized.
This comment has been minimized.
gterzian
May 29, 2020
•
Member
To answer your question: here you would use the OffThreadCompilationData to then queue a task, and inside that task you would do the rest such as calling FinishOffThreadScript. The task will execute "on the main thread".
Also, the JS context can be obtained from inside the task, by calling global.get_cx() and then you need to de-reference it when needed, for example by doing FinishOffThreadScript(*global.get_cx(), token);
(and yes context is often used in the other structs I pointed to to refer to the Trusted<GlobalScope> but that is just a name to say it's the "context" for the overal task that is being queued)
This comment has been minimized.
This comment has been minimized.
AbhishekSharma102
May 29, 2020
Author
Contributor
Like other structs this would also not have access to the original context, and needs to be stored. Is that correct?
This comment has been minimized.
This comment has been minimized.
gterzian
May 30, 2020
Member
Yes, correct, you need to store the Trusted<GlobalScope>(and other things if you need them.
| @@ -285,6 +285,14 @@ struct FileListener { | |||
| task_canceller: TaskCanceller, | |||
| } | |||
|
|
|||
| #[derive(JSTraceable, MallocSizeOf)] | |||
This comment has been minimized.
This comment has been minimized.
gterzian
May 29, 2020
Member
Since this struct will be used in the IPC router only, you don't need to derive those traits which are mostly meant for stuff inside a struct marked as dom_struct.
| pub struct OffThreadCompilationData { | ||
| context: JSContext | ||
| canceller: TaskCanceller, | ||
| task_source: TimerTaskSource, |
This comment has been minimized.
This comment has been minimized.
gterzian
May 29, 2020
•
Member
To answer your other question, so here you should instead store the NetworkingTaskSource, which is available via a call to GlobalScope.networking_task_source().
Then later when you need to queue a task, you can do OffThreadCompilationData.task_source.queue_with_canceller(or do this via a method on the struct), like is done for example at
servo/components/script/dom/globalscope.rs
Line 404 in d7d5645
This comment has been minimized.
This comment has been minimized.
gterzian
May 29, 2020
•
Member
I'd say you can define a handle(&self, token: *mut c_void) method on the struct, and then from the callback call handle(token), and inside the method queue the task and move the token into the task closure, then inside the closure you can use the token for example to call FinishOffThreadScript.
This comment has been minimized.
This comment has been minimized.
AbhishekSharma102
May 29, 2020
•
Author
Contributor
Once the FinishOffThreadScript has been called, we need to call evaluate_script_on_global_with_result with the obtained script, to call JS_ExecuteScript. So calling CompileOffThread can be called directly from run_a_classic_script.
Is that correct? If yes, how would the result be returned by evaluate_script_on_global_with_result to run_a_classic_script, as the task is queued and i think forgotten.
This comment has been minimized.
This comment has been minimized.
gterzian
May 30, 2020
•
Member
On second thoughts, I think we should do the off-thread compilation earlier, when the data has been received from the network.
So the entry point should I think be around here:
So I think you can:
add a new enum looking like:
SourceCode {
Text(DOMString),
Compiled(JSScript),
}Replace ScriptOrigin text with code: SourceCode.
- Store both the
Trusted<HTMLScriptElement>and theScriptOrigin, and alsoExternalScriptKindonto theOffThreadCompilationData. - Call the off-thread compilation API
- From the callback, queue a task that will:
a. callFinishOffThreadScript
b. SetScriptOrigin.codeto to the result of that call .
c. continue running the algorithm at
Then evaluate_script_on_global_with_result could be changed to take an SourceCode as the code argument, and inside you can either call Evaluate2 or JS_ExecuteScript, based on what is available.
I'm not completely sure if this is completely spec compliant, actually I'm not seeing anything about compilation in the spec, so I think this is a good starting point, and it seems to sort of match what Gecko is doing(since the code is in their ScriptLoader class). cc @jdm
This comment has been minimized.
This comment has been minimized.
AbhishekSharma102
May 31, 2020
•
Author
Contributor
Just a question, can these scripts be compiled in any context? As GlobalScope is not yet defined and would not be available.
Also how do we ensure in this that offthreadcompilation is used only when called by run_a_classic_script, as I believe this is done for all scripts?
This comment has been minimized.
This comment has been minimized.
gterzian
May 31, 2020
•
Member
o I think I will need to create a new task_canceller for this. Is it correct?
Yes you can call global.task_canceller() to get one, and then store it on the OffThreadCompilationContext along with the rest, and then use it later to queue the task. (see for example
servo/components/script/dom/globalscope.rs
Line 385 in 1a61937
This comment has been minimized.
This comment has been minimized.
AbhishekSharma102
May 31, 2020
Author
Contributor
I tried getting the globalscope object using self.elem.global() but wasn't able to. HTMLScriptElement doesn't seem to have it. Can you please confirm?
This comment has been minimized.
This comment has been minimized.
gterzian
May 31, 2020
Member
I think you first need to get the root out of the trusted, doing let elem = self.elem.root(); , then elem.global() should work
This comment has been minimized.
This comment has been minimized.
AbhishekSharma102
Jun 1, 2020
•
Author
Contributor
I had some questions regarding OffThreadCompilation methods we are using.
- These methods are written in C, but we do an unsafe call to them using a wrapper defined in rust?
- For JSScript, I get the error that malloc_size_of is not implemented. Upon reading the documentation, I believe it is similar to a string in rust for the time being. So implementing that would mean something to similar done for string?
- Another error during task queuing is OffThreadToken cannot be passed between threads safely. I think a simple clone should do the job, so that function needs to be implemented for it. Is that correct?
Also pushed some code. Please let me know if going in the correct direction.
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 3, 2020
•
Member
- Yes, I think that's a correct description.
- You can ignore it, using an annoation like in
servo/components/script/dom/globalscope.rs
Line 250 in 948dd73
- I'm not sure about this one. I would say define a
struct OffthreadToken;, and then in the callback cast the *mut OffThreadToken to it? Then cast it back when you callFinishOffThreadScript?
@jdm do you have an idea how to represent OffThreadToken* token in Rust and make it Send? It appears to be just an empty class defined at https://github.com/servo/mozjs/blob/aabcc9ba889b2755f1e4e83f28323a60415a790f/mozjs/js/src/vm/HelperThreads.h#L37
|
Could you please add a "FIX #{issue number}" in the opening description for the PR(that way it will be closed when this will, and it also makes it easier to go back to the issue). Looks like it's going in the right direction! |
| @@ -860,6 +860,12 @@ unsafe impl JSTraceable for StyleLocked<MediaList> { | |||
| } | |||
| } | |||
|
|
|||
| unsafe impl JSTraceable for JSScript { | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 1, 2020
Member
I think we're going to have to store the script in a Heap, and then this won't be necessary, see https://github.com/servo/rust-mozjs/blob/11cabdef2cbf0884a2cc33e3c73719646bd31ce5/src/rust.rs#L465
| ) { | ||
| let off_thread_compilation_context: &mut OffThreadCompilationContext = | ||
| &mut *(callback_data as *mut OffThreadCompilationContext); | ||
| let elem = off_thread_compilation_context.script_element.root(); |
This comment has been minimized.
This comment has been minimized.
| let _ = off_thread_compilation_context.task_source.queue_with_canceller( | ||
| task! (off_thread_compile_continue: move || { | ||
| let global_context = global; | ||
| off_thread_compilation_context.script_origin.code = SourceCode::Compiled(*FinishOffThreadScript(*global_context.get_cx(), token)); |
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 3, 2020
Member
So I think we're going to have to do two things:
- Root the
JsScriptas soon as it is received here, using therooted!macro(you'll see various example across the codebase, just search you'll find many). - store it inside a
Heapinside theScriptOrigin. For that I think you'll have to do something like:
let rooted_script = rooted! //
let heap = Heap::default();
heap.set(rooted_script.get());
let source_code = SourceCode::Compiled(heap);
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Ok so I ended-up looking at this locally, and I think it's actually pretty hard to find the right structure, I ended-up finding something over at https://github.com/gterzian/servo/tree/unblock_off_thread_compilation_pr, which I think could be useful to unblock this PR? It took me quite a lot of compilation cycles locally to figure out, so I think it would have been impractical to do with via even more, and probably confusing, review cycles. @AbhishekSharma102 How about you rebase this off of that branch? There is still some work to finish, for example in I think there is also an issue that is similar to this and you could use what you've learned to do that one, it is at #25209 |
|
Hey @gterzian ! Thanks for the help. I will take a look. Sorry this is taking so long, as pre occupied with some other work. |
|
I keep facing issues in this compilation step. with this note: Any suggestions on this? |
|
It might be #27090, or another issue related to gstreamer, or it might warrant opening a new issue. |
|
You will need to run |
|
@AbhishekSharma102 Ok thanks for adding the "execute script part", which looks good(although I suspect we'll need to debug it a bit in practice). On that last commit, you could add me a co-author, using 2792687+gterzian@users.noreply.github.com as email and following the instructions at https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors |
|
@gterzian I will do that. I am planning to squash commits into one, and will add into that. Regarding the squashing, unfortunately I merged from master after the first two commits, so there are several comments between the first and last two. Could you suggest on how to merge them? |
I'm not sure, maybe just try squashing them all into one? Usually we don't merge, rather we rebase(and only if there is an actual conflict or it's otherwise necessary), see https://github.com/servo/servo/wiki/Github-workflow |
I tried rebasing my branch off of servo/master and there were no conflicts, what problems are you encountering? |
00d436a
to
37358b5
|
Managed to resolve them. Using git too much for the first time, thus the issues. Merged all commits to one. Please let me know if any other changes/tests are needed. |
|
@bors-servo try=wpt |
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. -->
|
|
Co-authored-by: Gregory Terzian <2792687+gterzian@users.noreply.github.com> Co-authored-by: Abhishek Sharma <20724848+AbhishekSharma102@users.noreply.github.com>
|
@bors-servo r=jdm,gterzian |
|
|
I seem to have completely missed this. Sorry for this. |
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 retry |
|
|
|
@jdm thanks for catching that. You added the tracer right? So you mean previously it could be GC'ed during the off-thread compilation? |
|
Not GCed; we called |
|
I don't think I added the JSScript tracer to this PR. I guess we just haven't ever tried to store a JSScript in a Heap before? |
Makes sense, thanks for the info.
I think so too. There is this: https://github.com/servo/rust-mozjs/blob/0caf5549cddbb34ad16abf35fb6bfbff824a4d14/src/rust.rs#L466 However that's |
I added the tracer as RootedTraceableBox needed it i think. So did something similar to that was done for JSString and JSObject. Hope that was correct. Also, can you suggest a easy/medium issue that I could work on next? Should i pick up #25209 ? |
|
#25209 is a good one I think, yes. |
AbhishekSharma102 commentedMay 29, 2020
•
edited
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors