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

Performance improvements #49

Closed
trentgrover-wf opened this issue May 12, 2015 · 13 comments
Closed

Performance improvements #49

trentgrover-wf opened this issue May 12, 2015 · 13 comments

Comments

@trentgrover-wf
Copy link
Collaborator

We're seeing significant general performance degradation when moving JS based react components to Dart based react components. We will do some performance profiling to trace the most likely drain points, but @hleumas do you have any ideas for how to improve performance?

@hleumas
Copy link
Contributor

hleumas commented May 12, 2015

@trentgrover-wf I suspect dart:js interoperability layer as the cause, but I am not sure. If you could somehow improve the overall performance, it would be really awesome!

@jonaskello
Copy link

I'm also trying to use JS based react components inside Dart. @trentgrover-wf: Are you moving custom components or something available at http://react-components.com ?

@johnmccutchan
Copy link

@trentgrover-wf Could you quantify the slow down? What operations are slower?

Are there benchmarks that the react-dart team runs?

@trentgrover-wf
Copy link
Collaborator Author

@johnmccutchan I'm not aware of any routine benchmarks to use for comparison, but I'll share some more detailed observations and sample code later today. Unfortunately most of the analysis that I'm able to do by implementing identical simple React components in JS and Dart, then compare rendering frame rates and JS stack traces.

@trentgrover-wf
Copy link
Collaborator Author

@johnmccutchan I created a repo with a README detailing the issues I'm seeing. Most of the performance degradation appears to be in Dart JS interop overhead in the React render cycle. Any ideas?

https://github.com/trentgrover-wf/react-dart_perf

@hleumas
Copy link
Contributor

hleumas commented May 24, 2015

@trentgrover-wf Thank you very much for creating the benchmark. Maybe somebody from dart:js team would be able to help. I will ask @sethladd what does hi think about it.

@justinfagnani
Copy link

Ok, I took a quick look at the benchmark, specifically Dart-2.dart, and have some initial observations.

First, there's some incredibly fine-grained interop going on here - I see dozens of calls into Dart functions from JS per frame, usually leading to corresponding calls from Dart->JS. Crossing in either direction is potentially expensive, because all arguments and return values need to be converted or proxied (this involves setting properties to store the proxies through functions that are currently not optimizable in V8), but calls from JS->Dart are more expensive because Dart closures are wrapped by several functions in order to implement the conversions and get the calling convention correct.

This level of interop is never going to be fast, as least until we have wrapper-less interop with something like the Dev Compiler. The best strategy is to limit the number of language crossings, and limit their overhead by using no-arg functions and primitive arguments.

A few ways you might be able to limit crossings:

  • in _registerComponent don't call functions like _getInternal, _getComponent, or _React.callMethod('findDOMNode', [jsThis]) per-callback, but store them per component instance. I think this would mean moving most of the logic of _registerComponent to react.Component so that you can cache the JS objects in each Dart instance.
  • Re-implement selective parts of React in Dart rather than just wrap everything, in particular:
  • All the built-in element creation functions (react.a, etc) require a Dart->JS call. Since render() should be fast, this is probably very bad. Can these just be implemented in Dart?
  • Implement ReactComponent and ReactCompositeComponent in Dart, since they call into Dart functions frequently you might be able to move the JS->Dart crossing down the stack and avoid a lot of sequences like: JS->Dart, trivial JS logic, JS->Dart, etc.
  • Be careful that you're not re-implementing some features that dart:js might give you more efficient versions of:
  • Use new JsObject.jsify to convert Maps and Lists to JS.
  • Rather than tracking JS component -> Dart component identity via _getComponent try to rely on dart:js's identity tracking. I think this would be done by converting a Dart component to JS, then patching in callback methods as JS callbacks on the instance. When the Dart callbacks are called, this is correct, and you can get the JS 'this' with new JsObject.fromBrowserObject(this).
  • Be lazy when converting objects. Rather than eagerly converting events, require that Dart code convert the event when they ask for it, that way you don't pay for converting something never used.

Another potential issue is that the overall style of react.dart is going to be very hard for dart2js to optimize. I haven't looked at the output of --dump-info, but much of the API is just untyped variables that are later set to closures by one of the implementations. This type of code is very hard on dart2js, it'll be forced to include a lot of checks. For functions called in render() and the callbacks, this could add real overhead (though it's hard to tell right now). I'd consider trying to make the API more statically analyzable. Maybe define a class that represents the API, rather than using variables in a library. You can have different implementations of the API per environment, and if you do it right dart2js (probably one single assignment of the API at app startup) should be able to tell a lot more, and not include null, type and method exists checks.

Finally, this is my first look into React code, but it looks like all state is stored as Maps. Is this correct? In JavaScript there's a blurry performance line between class-like objects and map-like-objects, but in Dart classes are going to be much faster. I also can't tell if there's a lot of converting Maps between languages, but that will be expensive. I might look into splitting this library into two concerns: 1) Consuming React.js components in Dart, trying to minimize the amount of js-interop used during rendering, and 2) Defining Dart-only React-like components, that again try to minimize interop usage by moving more of the rendering pipeline into Dart.

I hope this helps. It's been a while since I worked on dart:js, but this should be mostly correct advice :)

@sethladd
Copy link

@justinfagnani thank you very much for the analysis!

@sethladd
Copy link

cc @kevmoo @jacob314

@hleumas
Copy link
Contributor

hleumas commented May 27, 2015

@justinfagnani Thank you very much for such a detailed analysis!

@hleumas
Copy link
Contributor

hleumas commented May 27, 2015

Link to ongoing discussion on dart-misc:
https://groups.google.com/a/dartlang.org/forum/#!topic/misc/_tQ_Jh69xRg

@greglittlefield-wf
Copy link
Collaborator

Just made a PR that switches over to use the new JS interop and makes other optimizations for performance improvements: #88.

@aaronlademann-wf
Copy link
Collaborator

@trentgrover-wf this can be closed when #88 is merged, IMO.

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

8 participants