Closure compile with sandbox cache #12

Merged
merged 17 commits into from Feb 4, 2013

Conversation

Projects
None yet
2 participants
@apg
Contributor

apg commented Feb 1, 2013

As I mentioned in a personal email, this significantly speeds up rendering by compiling templates to functions rather than to scripts that must be executed in an execution context. I've tested this within the app we're using toffee for and am reasonably confident that I have not screwed up anything with these changes.

For timing comparison (I don't have real benchmark numbers, just the test output timing):

NEW

Regular Engine: SUCCESS for 16 cold tests in 483ms (30.19ms/test)
Regular Engine: SUCCESS for 480 hot tests in 89ms (0.19ms/test)
Minimized (browser) Engine: SUCCESS for 16 cold tests in 1116ms (69.75ms/test)
Minimized (browser) Engine: SUCCESS for 480 hot tests in 77ms (0.16ms/test)

OLD

Regular Engine: SUCCESS for 16 cold tests in 522ms (32.63ms/test)
Regular Engine: SUCCESS for 480 hot tests in 819ms (1.71ms/test)
Minimized (browser) Engine: SUCCESS for 16 cold tests in 1131ms (70.69ms/test)
Minimized (browser) Engine: SUCCESS for 480 hot tests in 800ms (1.67ms/test)

That's pretty significant, though the test suite isn't insanely complex. In our app, we seem to go from ~30ms to about ~10ms (for example), on my local machine. We make lots of use of partials and includes, which has been a huge pain point for us.

@malgorithms

This comment has been minimized.

Show comment
Hide comment
@malgorithms

malgorithms Feb 2, 2013

Owner

hey @apgwoz -

This performance improvement is very exciting!

One thing: the unit tests that I have do a good job of testing the syntax of the toffee language, but they're not sweet at testing a couple other things, like how browser support works (as it doesn't simulate a browser client). It's been on my todo list to make the run_tests fire up an actual express3 server and then use someone's client library to visit a page and make sure browser-side publishing works.

The reason I'm bringing this up: I see two things that I think don't work right that aren't in the language unit test.

These may be quick fixes for you.

  1. Something broke with browser-side rendering. Here's the test for it.

Check out your branch, then:

cd test/express3/
coffee app.coffee

Then visit localhost:3033 and make sure you see 3 matching columns. (The middle and right will be green, showing they're matching the reference column.) With this change I see an error. I can help you hunt for this, but probably not till next week. Compare this to master and you'll see it usually works.

  1. consolidate.js support seems busted

This may actually be the same bug as 1. I'm not sure.

Quick background: consolidate.js is a library by the guy who made express. It's a library to wrap around other templating languages, which in itself isn't that useful. But one thing it does is run unit tests on all the included templating languages to make sure they obey the same protocols for pubbing, which is kind of a good thing. With this toffee branch it gets an error:

  toffee
    ✓ should support locals (292ms)
    ✓ should not cache by default (156ms)
    ✓ should support caching (35ms)
    1) should support rendering a string

How I got this error:

  1. checked out consolidate.js
  2. npm intalled your version of toffee
  3. ran make test in that directory; toffee failed to pass

And then I compared:
2. npm installed normal version of toffee
3. ran make test and it passed

Of course, whatever makes it fail both these steps 1 and 2 should make it into a full test suite, which I can try to do at some point. In the meantime, I check these things manually when there's a big change to toffee.

Owner

malgorithms commented Feb 2, 2013

hey @apgwoz -

This performance improvement is very exciting!

One thing: the unit tests that I have do a good job of testing the syntax of the toffee language, but they're not sweet at testing a couple other things, like how browser support works (as it doesn't simulate a browser client). It's been on my todo list to make the run_tests fire up an actual express3 server and then use someone's client library to visit a page and make sure browser-side publishing works.

The reason I'm bringing this up: I see two things that I think don't work right that aren't in the language unit test.

These may be quick fixes for you.

  1. Something broke with browser-side rendering. Here's the test for it.

Check out your branch, then:

cd test/express3/
coffee app.coffee

Then visit localhost:3033 and make sure you see 3 matching columns. (The middle and right will be green, showing they're matching the reference column.) With this change I see an error. I can help you hunt for this, but probably not till next week. Compare this to master and you'll see it usually works.

  1. consolidate.js support seems busted

This may actually be the same bug as 1. I'm not sure.

Quick background: consolidate.js is a library by the guy who made express. It's a library to wrap around other templating languages, which in itself isn't that useful. But one thing it does is run unit tests on all the included templating languages to make sure they obey the same protocols for pubbing, which is kind of a good thing. With this toffee branch it gets an error:

  toffee
    ✓ should support locals (292ms)
    ✓ should not cache by default (156ms)
    ✓ should support caching (35ms)
    1) should support rendering a string

How I got this error:

  1. checked out consolidate.js
  2. npm intalled your version of toffee
  3. ran make test in that directory; toffee failed to pass

And then I compared:
2. npm installed normal version of toffee
3. ran make test and it passed

Of course, whatever makes it fail both these steps 1 and 2 should make it into a full test suite, which I can try to do at some point. In the meantime, I check these things manually when there's a big change to toffee.

@apg

This comment has been minimized.

Show comment
Hide comment
@apg

apg Feb 2, 2013

Contributor

@malgorithms I'll get these tests passing and let you know. I was sure there were other things that you did to verify. :)

Contributor

apg commented Feb 2, 2013

@malgorithms I'll get these tests passing and let you know. I was sure there were other things that you did to verify. :)

* closure_compile_with_sandbox_cache - fix browser mode problems
  - app.coffee tests pass
  - consolidate.js tests pass
@apg

This comment has been minimized.

Show comment
Hide comment
@apg

apg Feb 4, 2013

Contributor

@malgorithms these tests now pass. There were three little things:

  1. For browserMode, you require that coffee be compiled with bare = false
  2. There was a TMPL = function() .. that need not exist in browserMode
  3. There's a weird indentation problem. See the # keep this comment in view.coffee. Multiline strings in coffee seem to have an issue in that their leading whitespace gets chopped up until the first in use column.
Contributor

apg commented Feb 4, 2013

@malgorithms these tests now pass. There were three little things:

  1. For browserMode, you require that coffee be compiled with bare = false
  2. There was a TMPL = function() .. that need not exist in browserMode
  3. There's a weird indentation problem. See the # keep this comment in view.coffee. Multiline strings in coffee seem to have an issue in that their leading whitespace gets chopped up until the first in use column.
@malgorithms

This comment has been minimized.

Show comment
Hide comment
@malgorithms

malgorithms Feb 4, 2013

Owner

hey @apgwoz -

I think re: num 3 they'd call this a feature of coffeescript. It was an annoyance for me somewhere else in the toffee project, too. I worked around it.

Cool that you have all this working.

This morning I got Toffee's testing framework improved. While I didn't get consolidate.js integrated into the test, I actually got browser testing working. now when you do this:

> cake build
> cake test

It does all the tests in run_tests which you're used to, including a new one which:

  1. fires up an express3 server at localhost:3033
  2. visits the server (using a browser-simulating module called zombie), letting the JS run
  3. makes sure everything rendered correctly in the "browser"
  4. if failure, leaves server running, so you can visit it in your own browser

Do you want to rebase with my latest master and try the tests? If they pass, I can accept accept the pull request and have this live today, and I'll push a new version of toffee to npm with your speed improvements.

-cc

Owner

malgorithms commented Feb 4, 2013

hey @apgwoz -

I think re: num 3 they'd call this a feature of coffeescript. It was an annoyance for me somewhere else in the toffee project, too. I worked around it.

Cool that you have all this working.

This morning I got Toffee's testing framework improved. While I didn't get consolidate.js integrated into the test, I actually got browser testing working. now when you do this:

> cake build
> cake test

It does all the tests in run_tests which you're used to, including a new one which:

  1. fires up an express3 server at localhost:3033
  2. visits the server (using a browser-simulating module called zombie), letting the JS run
  3. makes sure everything rendered correctly in the "browser"
  4. if failure, leaves server running, so you can visit it in your own browser

Do you want to rebase with my latest master and try the tests? If they pass, I can accept accept the pull request and have this live today, and I'll push a new version of toffee to npm with your speed improvements.

-cc

@apg

This comment has been minimized.

Show comment
Hide comment
@apg

apg Feb 4, 2013

Contributor

Hey @malgorithms,

I'll rebase now and let you know! Thanks!

On Mon, Feb 4, 2013 at 2:08 PM, Chris Coyne notifications@github.comwrote:

hey @apgwoz https://github.com/apgwoz -

I think re: num 3 they'd call this a feature of coffeescript. It was an
annoyance for me somewhere else in the toffee project, too. I worked around
it.

Cool that you have all this working.

This morning I got Toffee's testing framework improved. While I didn't get
consolidate.js integrated into the test, I actually got browser testing
working. now when you do this:

cake build
cake test

It does all the tests in run_tests which you're used to, including a new
one which:

  1. fires up an express3 server at localhost:3033
  2. visits the server (using a browser-simulating module called
    zombie), letting the JS run
  3. makes sure everything rendered correctly in the "browser"
  4. if failure, leaves server running, so you can visit it in your own
    browser

Do you want to rebase with my latest master and try the tests? If they
pass, I can accept accept the pull request and have this live today, and
I'll push a new version of toffee to npm with your speed improvements.

-cc


Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13093354.

http://www.apgwoz.com

Contributor

apg commented Feb 4, 2013

Hey @malgorithms,

I'll rebase now and let you know! Thanks!

On Mon, Feb 4, 2013 at 2:08 PM, Chris Coyne notifications@github.comwrote:

hey @apgwoz https://github.com/apgwoz -

I think re: num 3 they'd call this a feature of coffeescript. It was an
annoyance for me somewhere else in the toffee project, too. I worked around
it.

Cool that you have all this working.

This morning I got Toffee's testing framework improved. While I didn't get
consolidate.js integrated into the test, I actually got browser testing
working. now when you do this:

cake build
cake test

It does all the tests in run_tests which you're used to, including a new
one which:

  1. fires up an express3 server at localhost:3033
  2. visits the server (using a browser-simulating module called
    zombie), letting the JS run
  3. makes sure everything rendered correctly in the "browser"
  4. if failure, leaves server running, so you can visit it in your own
    browser

Do you want to rebase with my latest master and try the tests? If they
pass, I can accept accept the pull request and have this live today, and
I'll push a new version of toffee to npm with your speed improvements.

-cc


Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13093354.

http://www.apgwoz.com

@apg

This comment has been minimized.

Show comment
Hide comment
@apg

apg Feb 4, 2013

Contributor

Done and passing. cake test is a much nicer experience, though it's a lot of new dependencies!

Contributor

apg commented Feb 4, 2013

Done and passing. cake test is a much nicer experience, though it's a lot of new dependencies!

@malgorithms

This comment has been minimized.

Show comment
Hide comment
@malgorithms

malgorithms Feb 4, 2013

Owner

yeah, at least they're dev dependencies :-)

ok, working on the merge...

Owner

malgorithms commented Feb 4, 2013

yeah, at least they're dev dependencies :-)

ok, working on the merge...

malgorithms added a commit that referenced this pull request Feb 4, 2013

Merge pull request #12 from apgwoz/closure_compile_with_sandbox_cache
Closure compile with sandbox cache; significant speedups in cached toffee pages

@malgorithms malgorithms merged commit b154076 into malgorithms:master Feb 4, 2013

@malgorithms

This comment has been minimized.

Show comment
Hide comment
@malgorithms

malgorithms Feb 4, 2013

Owner

@apgwoz - voila, merged and npm published. toffee version 0.0.64 has your changes. thanks!

of course, you're the most active user of Toffee, so let me know if anything got funky as a result of these changes.

Owner

malgorithms commented Feb 4, 2013

@apgwoz - voila, merged and npm published. toffee version 0.0.64 has your changes. thanks!

of course, you're the most active user of Toffee, so let me know if anything got funky as a result of these changes.

@apg

This comment has been minimized.

Show comment
Hide comment
@apg

apg Feb 4, 2013

Contributor

Yays!

Thanks! -- we'll let you know what gets funky, but hopefully nothing!

On Mon, Feb 4, 2013 at 2:58 PM, Chris Coyne notifications@github.comwrote:

@apgwoz https://github.com/apgwoz - voila, merged and npm published.
toffee version 0.0.64 has your changes. thanks!

of course, you're the most active user of Toffee, so let me know if
anything got funky as a result of these changes.


Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13095910.

http://www.apgwoz.com

Contributor

apg commented Feb 4, 2013

Yays!

Thanks! -- we'll let you know what gets funky, but hopefully nothing!

On Mon, Feb 4, 2013 at 2:58 PM, Chris Coyne notifications@github.comwrote:

@apgwoz https://github.com/apgwoz - voila, merged and npm published.
toffee version 0.0.64 has your changes. thanks!

of course, you're the most active user of Toffee, so let me know if
anything got funky as a result of these changes.


Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13095910.

http://www.apgwoz.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment