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

Use new Array instead of Array for V8 optimization #6250

Merged
merged 3 commits into from
Sep 18, 2017

Conversation

pranavpr
Copy link
Contributor

@pranavpr pranavpr commented Sep 15, 2017

Q                       A
Fixed Issues Fixes #6233
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added/Pass? Yes
Spec Compliancy? Yes
License MIT
Doc PR
Any Dependency Changes?

Use new Array instead of Array for array initialization for V8 optimization to kick in.

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 15, 2017

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

@@ -1,3 +1,5 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems unrelated but ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was auto inserted when I ran make fix. I missed it while committing, my bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're in my PRs now, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it must be after @loganfsmyth's helpers PR - maybe modules/import related?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh woops, that's from 2801bfe, looks like we were using it in https://github.com/babel/babel/blob/master/packages/babel-runtime/scripts/build-dist.js#L63

Let me push a fix and remove these changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed f35cf81, so you can remove these changes from the PR now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR.

@hzoo hzoo added the PR: Polish 💅 A type of pull request used for our changelog categories label Sep 15, 2017
@jridgewell
Copy link
Member

Yup, that's what sparked this. But then they suggested we just switch call sites to always use new.

@jridgewell jridgewell merged commit e98bb3d into babel:master Sep 18, 2017
@hzoo
Copy link
Member

hzoo commented Sep 18, 2017

nice work @pranavpr 👌!

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8 can't optimize Array with initial size
7 participants