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

Improves the logic to import objects in helpers #10161

Merged
merged 7 commits into from Aug 8, 2019

Conversation

@ifsnow
Copy link
Contributor

commented Jul 4, 2019

Q                       A
Fixed Issues? No
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

In the case of react-native,

import React, { PureComponent } from 'react';

Compiled as below.

var _react = _interopRequireWildcard(_$$_REQUIRE(_dependencyMap[7], "react"));

The below logic(looping) is performed whenever _interopRequireWildcard with react is called.

var newObj = {};

if (obj != null) {
  for (var key in obj) {
    if (Object.prototype.hasOwnProperty.call(obj, key)) {
      var desc = Object.defineProperty && Object.getOwnPropertyDescriptor ? Object.getOwnPropertyDescriptor(obj, key) : {};

      if (desc.get || desc.set) {
        Object.defineProperty(newObj, key, desc);
      } else {
        newObj[key] = obj[key];
      }
    }
  }
}

Generally, We have a lot of components that imports react or react-native. I don't think it's good to check the same object repeatedly on every call. so, I thought it would be better to put cache logic in here.

I counted the number of how many loops are executed to start my production-level App. It may feel like, but it feels like my app is a little faster. :)

Version Count
Current 11,032
New (Cache) 128

I don't fully understand Babel architecture. Please let me know if I have any mistakes.

I hope this helps.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11288/

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11044/

@nicolo-ribaudo
Copy link
Member

left a comment

This makes it more compliant to the node implementation:

// main.mjs

import * as a from "./a.js";
import { aFromB } from "./b.mjs";

console.log(a === aFromB);
// m.mjs

import * as a from "./a.js";

export { a as aFromB };
// a.js

module.exports = { foo: 1 };
Show resolved Hide resolved packages/babel-helpers/src/helpers.js Outdated
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Note that this optimization would only work when using @babel/plugin-transform-runtime, otherwise the map is re-created in every file.

@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@nicolo-ribaudo Thanks for your feedback. I will try something better based on what you said.

@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@nicolo-ribaudo I've modified it to work well in the case you mentioned. Please let me know if there's anything else I need to consider.

@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@nicolo-ribaudo @Andarist @zloirock Thank you all for the feedback.

I realized that there might be no global object. https://github.com/webpack/webpack/blob/master/buildin/global.js#L16

In this case, I modified the cache to not work. I'm sure cache works well in modern development environments.

@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@ljharb Thank you for pointing out what I did not think of.

Show resolved Hide resolved packages/babel-helpers/src/helpers.js Outdated
@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@nicolo-ribaudo @jridgewell @Andarist @zloirock @ljharb @cpojer Do you have plans to merge this PR?

I'm sure this cache feature makes React Native apps run faster. The following table shows the time spent running js bundle at startup.

Device + Engine Current (ms) Cache (ms) Improving
Android 6.0 (32bit) + JSC 3,182 2,990 6%🔻
Android 7.0 (64bit) + JSC 1,118 1,089 2.6%🔻
Android 6.0 (32bit) + Hermes 1,230 964 21.7%🔻
Android 7.0 (64bit) + Hermes 403 367 9%🔻

The following cases are more effective.

  • Use many components
  • Use low performance devices
  • Use Hermes engine

I think we can improve the performance of many React Native apps without any effort. Please review it positively.

@ljharb

ljharb approved these changes Aug 8, 2019

@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@ifsnow On the CircleCI failure, please rebase on upstream master.

@JLHwung

JLHwung approved these changes Aug 8, 2019

@ifsnow ifsnow force-pushed the ifsnow:fix/improve-import-wild branch from 2e0b029 to 5c622ec Aug 8, 2019

@ifsnow

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@JLHwung Thanks for your feedback. Rebased my changes on upstream master to clear CircleCI failure. I think Travis CI failure is due to a build system issue (Network timeout).

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Restarted

@nicolo-ribaudo nicolo-ribaudo merged commit 9ec26a7 into babel:master Aug 8, 2019

5 checks passed

babel/repl REPL preview is available
Details
buildsize No significant change
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.86% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.