Skip to content

Conversation

chicoxyzzy
Copy link
Contributor

Prevent leaking arguments
Replace regexp splitting and symbols changing by iteration through array

@benvinegar
Copy link
Contributor

Cool. Thanks for the contribution.

I'm generally okay with this patch, and I think the code is cleaner afterwards anyways (by introducing wrap* helpers). When I get the opportunity I'll just poke at it a bit more to feel more confident that it doesn't change any behavior.

@benvinegar
Copy link
Contributor

@chicoxyzzy – some quick feedback.

Besides performance, Raven.js tries to keep its filesize low. And if these optimizations work, we're exchanging file size for performance (by switching [].slice.call for inline arg copying). So, I need to be sure that 1) they are actually faster, and if so, 2) those optimizations are worth the added filesize.

Keeping that in mind, here's the changes I'd like made:

  • Revert the arg copying from addPlugin. This function is called at most once per-page (if at all), and thus will likely never be optimized. It also iterates at most through 1-3 plugins (today).
  • Similarly, _logDebug is Raven's internal logging tool, and it is only used if logging is enabled (debug: true). Furthermore, Raven emits very few log messages. You should revert the arg copying here as well.
  • That leaves one place – wrapTimeFn. I think, in this instance, the optimization is valid. We're wrapping multiple built-in methods (setTimeout, setInterval, requestAnimationFrame) that could be called many, many times over the lifecycle of a page, and it ought to be as fast as possible. Keep the arg copying, but please add a comment explaining why this is done (e.g. link to the Bluebird article).

The other refactorings in this patch are fine. If you can make the changes above, I'll gladly merge.

cc @mattrobenolt

@chicoxyzzy chicoxyzzy force-pushed the optimisations branch 2 times, most recently from 24fc8bc to 4578d57 Compare April 7, 2016 22:27
@chicoxyzzy
Copy link
Contributor Author

I've finally broke my PR :(
I'll try to fix it

@benvinegar
Copy link
Contributor

Yeah, sorry ... I was heavily working on this code at the same time :(

@chicoxyzzy
Copy link
Contributor Author

Is it ok if I'll just create another one and close this one?

@chicoxyzzy chicoxyzzy closed this Apr 7, 2016
@chicoxyzzy chicoxyzzy deleted the optimisations branch April 7, 2016 23:10
@chicoxyzzy
Copy link
Contributor Author

recreated as #551

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

Successfully merging this pull request may close these issues.

2 participants