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

Application.renderToString performance #375

Open
swolfod opened this issue Aug 21, 2015 · 5 comments
Open

Application.renderToString performance #375

swolfod opened this issue Aug 21, 2015 · 5 comments

Comments

@swolfod
Copy link

swolfod commented Aug 21, 2015

I am building an isomorphic app upon marty and I noticed that Application.renderToString is not performing well and can easily take hundreds of milliseconds for simple pages.

By investigating the source code I found out that it was because in the internal helper function waitForFetches in renderToString.js file calls React.renderToString for every cycle it hits detecting unfinished fetches. Even if there are absolutely no fetches the React.renderToString function is called at least twice, and for every fetch one more redundant rendering is made even if the data is already local. React.renderToString is VERY expensive and calling it multiple times in one request can easily eat up cpu power.

Now I understand that 0.10 is the final major release for marty. But I really hope this problem can be fixed and the redundant renderings can be removed.

Thanks for the great job!

@taion
Copy link
Member

taion commented Aug 21, 2015

What do you propose? We use renderToString to figure out what fetches still need to be made - it's what walks down the component tree and ensures invocation of appropriate container methods.

@swolfod
Copy link
Author

swolfod commented Aug 21, 2015

I will need to dig deeper into the code to come up with a possible solution. However, some tentative ideas may include either detecting pending fetches without calling renderToString, or call it only once for each node in the tree and keep the result.

@taion
Copy link
Member

taion commented Aug 21, 2015

That's not quite right, BTW - renderToString kicks off a new cycle if it detects any new fetches have been made. Within any given render cycle, it won't end the cycle until all pending fetches have been resolved: https://github.com/martyjs/marty-lib/blob/v0.10.8/src/application/application.js#L167-L169

@swolfod
Copy link
Author

swolfod commented Aug 22, 2015

Can the renderToString result be buffered during detecting so if there is no new fetches the buffered result can be used instead of making another call of renderToString?

Also, per my test, the diagnositics look like this when the render is finished:
[{ status: 'DONE',
storeId: 'testStore1',
fetchId: 'testfetch_0',
time: 84,
result: ... },
{ status: 'PENDING',
storeId: 'testStore1',
fetchId: 'testfetch_0',
time: 155,
result: ... }]

While all the data in testStore1 is local and do not require remote fetch. Why it is marked 'PENDING' and therefore triggers a new cycle?

@goldensunliu
Copy link

I am experiencing the same.
even odder is if when your local fetch returns the value false, it never gets to status: 'DONE' and retry as many cycles as marty allows.

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

No branches or pull requests

3 participants