-
Notifications
You must be signed in to change notification settings - Fork 38
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
Runtime performance improvements #29
Conversation
@francescoagati @zabojad @kevinresol feedback welcome |
for me is good. Tomorrow i try to test in a project. |
I'll probably revert the |
Nevermind, |
yes object.assign isn't supported from many browsers |
@francescoagati I pushed a fix for dce; can you give me a minimal example for the other problem? |
for the sample tomorrow i try to create an example for the first error. |
i think the problem is when props are empty for example in
i have see that with createElement active the property props isn't null |
Good catch, it's fixed now. |
yeah now work all! and is really fast. Thanks really good work. now with inline of react elements and inline of haxe functions the rendering is really fast |
is really fast the initial rendering and also the updates |
Cool, is that really measurably faster for you? Did you profile it? |
not profiled but seeing the rendering startup the difference is visible |
founded some benchmarks for inlining. https://medium.com/doctolib-engineering/improve-react-performance-with-babel-16f1becfaa25#.4lzfezjm3 |
@francescoagati @zabojad @kevinresol added another optimization to generate more compact inline elements. There's an option to have the less compact but monomorphic variant using |
About monomorphic option, we've got this note from Facebook: "On the React team we've been operating under the assumption that it's better to include the nulls." |
i think that monomorphic are good for performance |
Yes it is "in theory" better, but nobody seems sure if for the particular case of inline react elements it will help a lot considering we are generating more code and creating more complex objects. |
read this comment on inferno infernojs/inferno#8 (comment) |
Anyway I also think it's good but we need numbers ;) |
@elsassph I tried the literal branch and it compiles fine with my codebase. And I didn't notice any runtime bug yet. I think the performance has been slightly improved on my side (because my rendering is not very heavy anyway), but yeah, just eyeballing, no numbers. I am not very sure how to profile react apps, any quick guides for that? |
For profiling you can typically use Chrome's Timeline in the debugging tool. |
ping @jeremyfa |
Conflicts: src/lib/api/react/ReactMacro.hx
…-createElement More compiler optimizations
@francescoagati @zabojad @kevinresol added a last safety (inlining breaks components which need accessing the context): inlining should not happen for extern React component (should resolve problems with react-router) and you can prevent it manually by adding |
First tests on a little Apache Cordova project with this branch + react 15 and everything seems to work just fine in both dev and prod modes... It's a little project involving react-router and react-redux... I'm gonna try on a much larger project within the next 7 days... |
Ok I'll merge then - I'm tagging current master as 0.4.0 so you can keep pointing on it if needed. |
Note: when using `for` control structure, you still need to handle keys yourself. Fixes massive-oss#29
Issue #28 suggests to use the "inline react elements" optimization for production code.
cf. facebook/react#3228
This PR enables the literal objects optimization when building in release mode.
It can be disabled by setting
-D react_no_inline
.This optimization affects both JSX code AND direct uses of
React.createElement
.Note:
ref
with aString
value (in practice we're deoptimizing unless we are sure theref
is a function); it requires the fullcreateElement
context,@:reactContext
meta to the component class,Object.assign
instead ofReact.__spread
as deprecated by React v15.Additional changes:
-D react_hot
): in debug mode, adds a__source
property toReactComponent
classes,displayName
is now only generated in release mode.