Skip to content

Use new Array instead of Array for V8 optimization#6250

Merged
jridgewell merged 3 commits intobabel:masterfrom
pranavpr:master
Sep 18, 2017
Merged

Use new Array instead of Array for V8 optimization#6250
jridgewell merged 3 commits intobabel:masterfrom
pranavpr:master

Conversation

@pranavpr
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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