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

Do Background Processing to Determine Performance #139

Closed
jeresig opened this issue May 2, 2012 · 8 comments
Closed

Do Background Processing to Determine Performance #139

jeresig opened this issue May 2, 2012 · 8 comments
Milestone

Comments

@jeresig
Copy link
Member

jeresig commented May 2, 2012

Right now infinite loops (or incredibly complex programs) will just cause the web page to hang. The only viable solution is to run a neutered version of the code inside a web worker to make sure that it won't hang forever (or, at least, a very long time). We can move much of the code execution logic (and JSHint) into there, which will help, at least.

We should determine what a good threshold is. 1 second of run time? If we're expecting instant compilation we're going to have to be pretty aggressive about this.

@jlfwong
Copy link
Contributor

jlfwong commented May 14, 2012

While I'm waiting on review for the slug support stuff, I'm taking a look into this.

@jlfwong
Copy link
Contributor

jlfwong commented May 14, 2012

So I have it (mostly) working, but the structure of a whole bunch of code needs to change because this fundamentally makes Output.exec async - I need to run it in the background thread and see if it completes.

This is especially bad because there are a bunch of callsites to Output.exec in unused code - namely TextOutput.

@jeresig
Copy link
Member Author

jeresig commented May 15, 2012

So a few thoughts on this first pass-through:

  • I think it'd make sense to move the logic that handles the worker spawning, etc. into CanvasOutput.runCode instead of Output.exec (since exec is used pretty much everywhere - and we don't need to make all execution async, just the detection of long-running code).

I think almost all of the web worker stuff can be moved into this particular call:

    // TODO: Make sure these calls don't have a side effect
    for ( var global in Output.globals ) (function( global ) {
        grabAll[ global ] = typeof Output.context[ global ] === "function" ?
            function(){ fnCalls.push([ global, arguments ]); } :
            Output.context[ global ];
    })( global );

    Output.exec( userCode, grabAll );

We can kill two birds with one stone: Collect a list of all the function calls that occur with their arguments (what happens now) and get a count of how many function calls occur (which needs to occur). I would then move the rest of runCode() to occur after the viability of the code has been determined.

  • So there are two problems happening here, one of which you pointed out: The calls that we're monitoring aren't taking as long as the real drawing operation and the drawing operations may not be occurring right when the code is first executed - in fact the execution may happen later when the draw() function is called. I think the only really viable option that we have is to watch for X functions calls per draw() loop OR timeout after Y milliseconds, whichever happens first. The biggest issue with this technique is that it doesn't differentiate between calls to ellipse() and calls to sin() but maybe we can find ways around that.
  • I bet we can get away with only creating a single web worker that we can re-use again and again, might save us some overhead.

@jlfwong
Copy link
Contributor

jlfwong commented May 15, 2012

As for the first point, I am actually calling the draw function: https://github.com/phleet/canvas-editor/blob/cb74ee4e56a21920406ec06e2d7e5929659b8c0a/js/worker.js#L34.

This could be done alternatively by modifying code before we actually run it in the worker by just appending for (var i = 0; i < 10; i++) draw();, which is probably easier now that I think about it

@jeresig
Copy link
Member Author

jeresig commented May 15, 2012

Oh! Yeah, that's not a bad idea, actually.

@jlfwong
Copy link
Contributor

jlfwong commented May 15, 2012

Actually, appending (function() { for (var i = 0; i < 10; i++) draw(); })() would be better so we don't possibly trample a pre-existing variable i.

@jeresig
Copy link
Member Author

jeresig commented May 15, 2012

Naturally.

@jeresig
Copy link
Member Author

jeresig commented May 25, 2012

Fixed over here: http://phabricator.khanacademy.org/D105

@jeresig jeresig closed this as completed May 25, 2012
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