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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX beta] property_set should reuse reference to its meta #15210

Merged
merged 4 commits into from
May 5, 2017
Merged

[BUGFIX beta] property_set should reuse reference to its meta #15210

merged 4 commits into from
May 5, 2017

Conversation

stefanpenner
Copy link
Member

No description provided.

@scalvert
Copy link
Contributor

scalvert commented May 5, 2017

Good catch! No peeking :)

runspired
runspired reviewed May 5, 2017
View reviewed changes
packages/ember-runtime/lib/mixins/array.js Outdated
for (let key in keys) {
meta = meta || peakMeta(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a misspelling ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, should be peekMeta

Copy link
Contributor

Choose a reason for hiding this comment

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

although I do like that we've hit "peak meta"

runspired
runspired reviewed May 5, 2017
View reviewed changes
packages/ember-runtime/lib/mixins/array.js Outdated
for (let key in keys) {
meta = meta || peakMeta(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, peekMeta

runspired
runspired suggested changes May 5, 2017
View reviewed changes
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Fix da peak

@stefanpenner
Copy link
Member Author

stefanpenner commented May 5, 2017 •
edited
Loading

@bmeurer this should eat into the 5%+ of time spent in weakmap lookup you guys have been seeing.

@stefanpenner
Copy link
Member Author

@homu r+

@homu
Copy link
Contributor

homu commented May 5, 2017

📌 Commit a7ab514 has been approved by stefanpenner

homu added a commit that referenced this pull request May 5, 2017
@homu
Auto merge of #15210 - emberjs:reuse-meta, r=stefanpenner
22804ea 
[BUGFIX beta] property_set should reuse reference to its meta
@homu
Copy link
Contributor

homu commented May 5, 2017

⌛ Testing commit a7ab514 with merge 22804ea...

@stefanpenner
Copy link
Member Author

@homu retry

@homu
Copy link
Contributor

homu commented May 5, 2017

⌛ Testing commit a7ab514 with merge 4e868e9...

homu added a commit that referenced this pull request May 5, 2017
@homu
Auto merge of #15210 - emberjs:reuse-meta, r=stefanpenner
4e868e9 
[BUGFIX beta] property_set should reuse reference to its meta
@stefanpenner
Copy link
Member Author

@homu what's taking you so long :(

@rwjblue
Copy link
Member

rwjblue commented May 5, 2017

I think homu is basically broken here :(

@rwjblue rwjblue merged commit fe136c8 into master May 5, 2017
@rwjblue rwjblue deleted the reuse-meta branch May 5, 2017 23:52
@homu homu mentioned this pull request May 6, 2017
Closed
@bmeurer
Copy link
Contributor

bmeurer commented May 7, 2017

I don't have the full context on this change.

@stefanpenner
Copy link
Member Author

Sorry. This pull request removes repeated and unnecessary weakmap get's... I realize you guys are also working on improving weakmap perf so I figure I would loop you in.

TL;DR this pr reduces the number of WeakMapGets's to at least 1/3 in some cases.

@bmeurer
Copy link
Contributor

bmeurer commented May 8, 2017

Ah I see. That makes total sense, no matter what V8 (or any other engine) is doing.

@stefanpenner
Copy link
Member Author

@bmeurer yup, should work in combination with any work engines do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@runspired runspired runspired approved these changes

@krisselden krisselden krisselden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@stefanpenner @scalvert @homu @rwjblue @bmeurer @krisselden @runspired