Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Move Realm creation to another thread #433

Closed
jasonwilliams opened this issue May 31, 2020 · 10 comments
Closed

Move Realm creation to another thread #433

jasonwilliams opened this issue May 31, 2020 · 10 comments
Labels
discussion Issues needing more discussion enhancement New feature or request help wanted Extra attention is needed performance Performance related changes and issues

Comments

@jasonwilliams
Copy link
Member

Realm creation is currently quite slow, and lexing and parsing doesn't start until this is finished. In the screenshot below we can see that Realm::create() runs on the main thread, and blocks everything else until its finished.

I think further optimisations can be done here to:

  • Speed up set_field() which is where most of the time is being spent
  • put the &global env into a Mutax and create each of these objects concurrently

image

I'm also happy to discuss any other ideas or opportunities here

@jasonwilliams jasonwilliams added enhancement New feature or request help wanted Extra attention is needed performance Performance related changes and issues discussion Issues needing more discussion labels May 31, 2020
@brian-gavin
Copy link
Contributor

Have a few more ideas relating to throwing some of the builtin construction into threads, beyond just Realm::create()

Firstly, in regard to

put the &global env into a Mutax and create each of these objects concurrently

To prevent the overhead of accessing the global object via a Mutex during execution of JS code, it might be better to have the threads that create the builtins instead send the object they create to the main thread via a channel, so the main thread will be the only thread that mutates the global env.

This would also require updating every one of the init functions to not modify the global env - but this could be tricky, considering global is interior-mutated and the main thread writing to it may run in line with another thread having a read-only borrow done on it (such as to access an object for a prototype) which would cause a panic.

Maybe to solve this, the worker threads can send construct them concurrently, but the main thread will add them only after they are all finished.

But, this also presents a problem: some builtins need certain objects to exist during their own construction. For example, constructors that require the Function global object as their prototype. Maybe the solution to this would be to do required objects first, then the other ones in parallel afterwards.


Next, if these objects are constructed concurrently, a further optimization might be to have a thread initialize groups of them so each thread has similar amounts of work.

For example, one thread each for array, math, string because these are pretty big.
One thread to do all of bigint, boolean, etc. because each of these are pretty small, so it can accomplish all of them in (theoretically) the same time one thread will accomplish array.

Might be an optimization of an optimization, and it would require testing and benchmarking at each step.

I think it would be good to have the simple solution first, 1 thread per builtin, and try to see how that measures up to grouping up a few of them, since thread construction isn't free.

@Razican
Copy link
Member

Razican commented Jun 1, 2020

At some point, execution could also be done in streaming, in which case, the realm would need to be ready when the execution starts (which would call the parser streamer).

@jasonwilliams jasonwilliams linked a pull request Jun 1, 2020 that will close this issue
8 tasks
@jasonwilliams
Copy link
Member Author

jasonwilliams commented Jun 1, 2020

I've linked #419 with this because the optimisations in that should bring the realm creation time down significantly.

Once that's done I will do another measure to see the impact
Edit: I think the PR being done will close this issue so removed link but reference remains

@jasonwilliams jasonwilliams removed a link to a pull request Jun 1, 2020
8 tasks
@RageKnify
Copy link
Member

I went back to the May 31st commit to see how the code looked back then and since then there was the Value Refactor by @HalidOdat #498 which sped up objects and consequently field access by a lot.

Looking at https://boa-dev.github.io/boa/dev/bench/ it seems Realm Creation has become considerably faster than it used to be, keeping this up in case in the future we want to introduce multi-threading once the builtins are more developed?

@Razican
Copy link
Member

Razican commented Aug 25, 2020

The thing is that currently, we first tokenize + parse the whole program, and then we start creating the Realm. This creation is faster, but it could be parallelized with the lexing + parsing. We need it even if we fail to lex/parse, since we need to generate a Syntax Error.

@HalidOdat
Copy link
Member

The problem with trying to implement a parallel initialization for Realm is that a lot of them depend on each other, even if that was not the case when we create a new Gc<> it is stored in a thread_local so Gc<> is not Sync or Send so we cant pass them through threads. we might be able to initialize the Realm in the main thread while we parse/lex the code.

@jasonwilliams
Copy link
Member Author

Parsing and lexing should be independent of realm setup as neither of those tasks require use of the Gc, it’s only when you’re ready to execute it becomes a problem.

The thing is that currently, we first tokenize + parse the whole program, and then we start creating the Realm.

Has this changed?
Last I checked realm setup happens first then parsing starts once’s it’s built. https://boa-dev.github.io/2020/07/03/boa-release-09.html

@Razican
Copy link
Member

Razican commented Aug 29, 2020

Has this changed?
Last I checked realm setup happens first then parsing starts once’s it’s built. https://boa-dev.github.io/2020/07/03/boa-release-09.html

Not that I'm aware of, I might have mixed up the order, but the thing is that one waits for the other.

@jasonwilliams
Copy link
Member Author

jasonwilliams commented Sep 30, 2020

I wonder if for now we can change the order and do the parsing before realm creation.
That way if there's an early error we don't need to bother building the realm.

image

@Razican
Copy link
Member

Razican commented Oct 1, 2020

We still need the realm to create a SyntaxError if the parsing goes wrong, right?

@boa-dev boa-dev locked and limited conversation to collaborators Aug 29, 2021
@Razican Razican closed this as completed Aug 29, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
discussion Issues needing more discussion enhancement New feature or request help wanted Extra attention is needed performance Performance related changes and issues
Projects
None yet
Development

No branches or pull requests

5 participants