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

Remove Kernel#caller patches #36

Closed
wied03 opened this issue Sep 16, 2015 · 6 comments
Closed

Remove Kernel#caller patches #36

wied03 opened this issue Sep 16, 2015 · 6 comments
Labels
Milestone

Comments

@wied03
Copy link
Contributor

wied03 commented Sep 16, 2015

Karma-opal-rspec is now using stacktrace.js:

This issue should now:

  • Remove the Kernel#caller override in opal-rspec and just put in pure, raw JS stack traces only when a test fails and point people elsewhere (wherever this github issue gets done) to use source maps. Better yet, don't include any location info to avoid the overhead (see next bullet)
  • Compiler/Line Number Metadata #57 might be a more performant way of gathering metadata for every test
@wied03
Copy link
Contributor Author

wied03 commented Oct 27, 2015

@elia - Do you think this belongs in a separate GEM? These Bower dependencies can be pulled in via rails-assets (I use that to represent Bower deps as GEMs on my project) but I think this probably belongs in a separate GEM.

@elia
Copy link
Member

elia commented Oct 27, 2015

@wied03 I'm quite sure this should be optional, not necessarily by using another gem but one shouldn't be forced in. Although an argument in favor of an external gem can be made by pointing out that the gem could be independent from RSpec and just provide source-mapped backtraces to opal on nodejs.

@wied03 wied03 added this to the 0.7 Release milestone Jan 22, 2016
@wied03 wied03 modified the milestones: 0.6 Release, 0.7 Release Mar 10, 2016
@wied03 wied03 changed the title Source map support in results Remove Kernel#caller patches Mar 10, 2016
@wied03
Copy link
Contributor Author

wied03 commented Apr 20, 2016

Some Kernel#caller discussion here - opal/opal#894

wied03 added a commit that referenced this issue Apr 20, 2016
wied03 added a commit that referenced this issue Apr 20, 2016
wied03 added a commit that referenced this issue Apr 20, 2016
@wied03
Copy link
Contributor Author

wied03 commented Apr 20, 2016

It probably needs to be removed anyways because it's poorly tested and brittle, but removing this code in the kernel_caller_fix branch didn't improve performance that much:
image

@wied03
Copy link
Contributor Author

wied03 commented Apr 20, 2016

Test was conducted using time bundle exec rake opal_specs. Around 40 of the 140 tests are expected to fail so they will have exceptions regardless. I would have expected the other 100 tests that pass to improve these results more than they did.

wied03 added a commit that referenced this issue Apr 20, 2016
@wied03 wied03 closed this as completed Apr 20, 2016
@jgaskins
Copy link

Just throwing it in here for posterity that the majority of time spent in RSpec overhead (that is, not the actual specs themselves) was due to break calls in Enumerable.

Considering the results in opal/opal#1450, you can probably multiply any benefit gained by at least 4x. Possibly a lot more, considering that timing the Rake task measures time between command prompts, which includes loading node, Opal, and RSpec. This is a huge portion of execution time, too. For example, Clearwater specs take about 8 seconds on my machine, but nearly all of that is load time.

A better benchmark would probably be the execution time of the specs only.

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

3 participants