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

Investigate moving bundling into a worker #610

Closed
lottamus opened this issue Sep 30, 2020 · 11 comments
Closed

Investigate moving bundling into a worker #610

lottamus opened this issue Sep 30, 2020 · 11 comments
Labels
Milestone

Comments

@lottamus
Copy link
Contributor

  • Do we need this? Are we going to hit performance issues on the main thread?
  • How do we do it without requiring users to use webpack?
  • Can we make it work with the raw React distribution?
@lottamus lottamus added the chore label Sep 30, 2020
@lottamus lottamus added this to the 2020-09-30 milestone Sep 30, 2020
@P0lip
Copy link
Contributor

P0lip commented Sep 30, 2020

Do we need this? Are we going to hit performance issues on the main thread?

yes, for large (say Stripe-sized) documents with plenty of inline $refs or Redfish-like crap with plenty of external $refs (resolving such a document may last seconds)
When I measured it in Studio, it took over a 1s on my machine (my CPU is i7-10875H)

image

How do we do it without requiring users to use webpack?

we can inline a worker when bundling our own code - this is what we did in case of JSV back in the day

Can we make it work with the raw React distribution?

yes, if we inline the worker's code

You can find some of the answers here as well #575 (comment)

@marcelltoth marcelltoth modified the milestones: 2020-09-30, 2020-10-20 Oct 20, 2020
@P0lip
Copy link
Contributor

P0lip commented Nov 13, 2020

Tab hangs :P
image
image
STR:

  • import stripe spec
  • open v1/3d_secure path
  • click preview

bang, tab freezes

@P0lip
Copy link
Contributor

P0lip commented Nov 13, 2020

I can take care of it, as this kind of goes in our way when trying out some YJS stuff.
@lottamus @marbemac @marcelltoth you good with it?

@marbemac
Copy link
Contributor

🤔 why does the tab hang? There are not even any remote $refs in the stripe spec. How long does dereferencing take?

@marcelltoth
Copy link
Contributor

marcelltoth commented Nov 13, 2020

@lottamus @marbemac @marcelltoth you good with it?

If you confirm we have done everything we could - in a reasonable timeframe - to speed up json-schema-ref-parser in the first place, then I guess it's the way to go.

I just want to avoid "throwing hardware" at a problem that could simply be optimized.

@P0lip
Copy link
Contributor

P0lip commented Nov 13, 2020

I just want to avoid "throwing hardware" at a problem that could simply be optimized.

Well, same. That's why I questioned the addition of json-schema-ref-parser in first place.
Studio does not need it and it's unnecessarily slowed down by the redundant resolving process.

We can try incorporating APIDevTools/json-schema-ref-parser#195 but still, it's going to take some time regardless. As said, the actual solution would be to avoid the dereferencing entirely.

@P0lip
Copy link
Contributor

P0lip commented Nov 13, 2020

FWIW, 195 improves the situation. At least the tab no longer freezes.

@marcelltoth
Copy link
Contributor

marcelltoth commented Nov 13, 2020

Then let's try wrapping up and incorporating that first. If the problem still persists we'll think about other options.

By the way the entire problem is going to go away once we release the new TryIt. It drops the prism dependency completely and will only support mocking for Stoplight-hosted projects.

AFAIK the only thing that really needs dereferencing in elements is the RequestMaker because of this prism-based mocking. If we can drop prism, we can also drop the requirement that Docs is fed with dereferenced data -> problem gone.

This also supports that a short term solution (like the PR you mention) would be enough, and investing a lot of work into workerifying things is not worth it.

@marcelltoth
Copy link
Contributor

By the way in the studio use-case dereferencing actually takes place inside studio. So you can shove it into a worker in any way you'd like. The only place where elements does any dereferencing is inside its public-facing StoplightProject and API components, neither of which are used in platform-internal.

So I'm closing this issue, there's nothing we can do about the studio problem at the elements level and we are working to eliminate the requirement completely with #640 anyway.

@marbemac
Copy link
Contributor

and investing a lot of work into workerifying things is not worth it.

I agree with this and the rest of it 👍.

@P0lip
Copy link
Contributor

P0lip commented Nov 13, 2020

By the way the entire problem is going to go away once we release the new TryIt. It drops the prism dependency completely and will only support mocking for Stoplight-hosted projects.

AFAIK the only thing that really needs dereferencing in elements is the RequestMaker because of this prism-based mocking. If we can drop prism, we can also drop the requirement that Docs is fed with dereferenced data -> problem gone.

Oh yeah, totally. If that is going away, we won't need to do anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants