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

Improve performance #12

Open
jhnns opened this issue Mar 8, 2014 · 4 comments
Open

Improve performance #12

jhnns opened this issue Mar 8, 2014 · 4 comments

Comments

@jhnns
Copy link

jhnns commented Mar 8, 2014

I'm using review for developing an interactive widget. The widget is designed to run on different screen sizes and different types of data (variable amount of columns). That's why I need to keep an eye on ~15 states.

review is a good support for this task, but unfortunately it takes quite some time to reload 15 screenshots. After digging into the source code I saw that review spawns a child_process for every image. This adds ~2 seconds delay on every image.

I thought about ways how multiple screenshots could be rendered more efficiently:

  • Maybe there is the possibility to keep the process running. Thus we just need to tell the process to reload a given site.
  • We could also render different resolutions by just resizing the viewport. This of course should only be optional as it might not be intended. Additionally there should be a delay because maybe there are some scripts that need to be executed on resize.

What do you think?

@juliangruber
Copy link
Owner

When I last tried long running phantomjs processes they crashed too often on me, might not be an issue any more though. Then we could spawn n procs and have a work queue for new screenshots.

I'm down with trying that but don't have the time right now.

@jhnns
Copy link
Author

jhnns commented Mar 8, 2014

I could give it a try though I'm not too experienced with sub processes and work queues. How would you communicate between the processes? As far as I know there is no stdin available in phantomjs.

But we could spin up a webserver and communicate via localhost. I think that's exactly what phantomjs-wrapper does.

What do you think, should I give it a try using phantomjs-wrapper and open a pull-request?

@jhnns
Copy link
Author

jhnns commented Mar 8, 2014

And what do you think about just resizing the page? I think this is the way you'd usually test your webpages manually. Why shouldn't review speed up the rendering process by doing it exactly the same way? As I mentioned, it should be opt-out if reloading the page is necessary (but I'd turn it on by default).

@juliangruber
Copy link
Owner

Yeah I think http should be the way to go. And for resizing, SGTM :)

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

2 participants