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

Runtime performance improvements #29

Merged
merged 14 commits into from May 27, 2016
Merged

Runtime performance improvements #29

merged 14 commits into from May 27, 2016

Conversation

elsassph
Copy link
Contributor

@elsassph elsassph commented Apr 17, 2016

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:

  • there is an automatic de-optimization when using ref with a String value (in practice we're deoptimizing unless we are sure the ref is a function); it requires the full createElement context,
  • you can force de-optimization for components which require the React context by adding @:reactContext meta to the component class,
  • possible breaking change: now using Object.assign instead of React.__spread as deprecated by React v15.

Additional changes:

  • React Hot Reload 3 tagging option (-D react_hot): in debug mode, adds a __source property to ReactComponent classes,
  • displayName is now only generated in release mode.

@elsassph
Copy link
Contributor Author

@francescoagati @zabojad @kevinresol feedback welcome

@francescoagati
Copy link
Contributor

for me is good. Tomorrow i try to test in a project.

@elsassph
Copy link
Contributor Author

elsassph commented Apr 17, 2016

I'll probably revert the Object.assign change, __spread still works in React v15 and it would require a polyfill.

@elsassph
Copy link
Contributor Author

Nevermind, Object.assign is staying: React v15 just maps __spread to Object.assign so the polyfil will be needed anyway for IE and mobile...

@francescoagati
Copy link
Contributor

yes object.assign isn't supported from many browsers

@francescoagati
Copy link
Contributor

francescoagati commented Apr 18, 2016

i get two errors. One is this with props and dangerouslySetInnerHTML
image

the seconds is with dce full i get Uncaught ReferenceError: $$tre is not defined.
$$tre is eliminated from dce

@elsassph
Copy link
Contributor Author

elsassph commented Apr 18, 2016

@francescoagati I pushed a fix for dce; can you give me a minimal example for the other problem?
I could use dangerouslySetInnerHTML without problem.

@francescoagati
Copy link
Contributor

for the sample tomorrow i try to create an example for the first error.

@francescoagati
Copy link
Contributor

i think the problem is when props are empty for example in


{ '$$typeof' : $$tre, type : "hr", props : null, key : null, ref : null},blocks]}, key : null, ref : null};

i have see that with createElement active the property props isn't null

@elsassph
Copy link
Contributor Author

Good catch, it's fixed now.

@francescoagati
Copy link
Contributor

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

@francescoagati
Copy link
Contributor

is really fast the initial rendering and also the updates

@elsassph
Copy link
Contributor Author

Cool, is that really measurably faster for you? Did you profile it?

@francescoagati
Copy link
Contributor

francescoagati commented Apr 19, 2016

not profiled but seeing the rendering startup the difference is visible

@francescoagati
Copy link
Contributor

@elsassph
Copy link
Contributor Author

@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 -D react_monomorphic.
Can someone do some actual profiling to see if there is a difference when using compact VS monomorphic? Plus some numbers about the literal syntax improvements would be nice to have.

@elsassph
Copy link
Contributor Author

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."

@francescoagati
Copy link
Contributor

i think that monomorphic are good for performance
http://mrale.ph/blog/2015/01/11/whats-up-with-monomorphism.html

@elsassph
Copy link
Contributor Author

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.

@francescoagati
Copy link
Contributor

read this comment on inferno infernojs/inferno#8 (comment)
inferno is similar to react and use monomorphic objects in template

@elsassph
Copy link
Contributor Author

Anyway I also think it's good but we need numbers ;)

@kevinresol
Copy link
Contributor

@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?

@elsassph
Copy link
Contributor Author

For profiling you can typically use Chrome's Timeline in the debugging tool.

@elsassph
Copy link
Contributor Author

ping @jeremyfa

@elsassph elsassph changed the title Runtime performance improvement by generating react literals Runtime performance improvements Apr 28, 2016
@elsassph
Copy link
Contributor Author

elsassph commented May 21, 2016

@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 @:reactContext. I didn't try with redux; maybe you'd have a better suggestion for heuristics here.

@zabojad
Copy link

zabojad commented May 26, 2016

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...

@elsassph
Copy link
Contributor Author

Ok I'll merge then - I'm tagging current master as 0.4.0 so you can keep pointing on it if needed.

@elsassph elsassph merged commit 36be59c into master May 27, 2016
@elsassph elsassph deleted the feature/react-literals branch May 27, 2016 08:00
kevinresol pushed a commit to haxe-react/haxe-react that referenced this pull request Jan 29, 2020
Note: when using `for` control structure, you still need to handle
keys yourself.

Fixes massive-oss#29
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.

None yet

4 participants