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

Changed Attributes #121

Closed
wants to merge 1 commit into from
Closed

Conversation

jridgewell
Copy link
Contributor

By returning the changed attributes, we're able to avoid a second #apply.

Re: #85 (comment)
Re: #120 (comment)

@jridgewell
Copy link
Contributor Author

Did some digging into inlining, seems changedAttributes won't be inlined do to the #apply call, but updateAttributes and everything it does gets inlined into elementOpen:

Inlined getData called from updateAttribute.
# snip...
Inlined updateAttribute called from updateAttributes.
Inlined updateAttributes called from exports.elementOpen.

For reference, here's the full log.

By returning the changed attributes, we're able to avoid a second `#apply`.

Re: google#85 (comment)
@jridgewell
Copy link
Contributor Author

Pinging this again. Not seeing any difference on the creation or selection performance tests.

@sparhami
Copy link
Contributor

It is a bit confusing, but the perf tests use the -min.js from the dist folder (generated by js-min). I tried running those and I definitely do see a difference between your changes and the current state.

@jridgewell
Copy link
Contributor Author

Yup, I run gulp js-min before the each test, one for master and one for the changes. I'll rerun on another computer to see if I get anything different.

@jridgewell
Copy link
Contributor Author

Browser Test Branch Result (ms)
Chrome Creation Master 3.2
Chrome Creation changedAttributes 3.2
Chrome Selection Master 0.32
Chrome Selection changedAttributes 0.32
FireFox Creation Master 8.2
FireFox Creation changedAttributes 7.4
FireFox Selection Master 0.64
FireFox Selection changedAttributes 0.63

@sparhami
Copy link
Contributor

sparhami commented Nov 6, 2015

I tried this a while ago and wasn't able to reproduce your results. I don't plan on changing this for now.

@sparhami sparhami closed this Nov 6, 2015
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.

2 participants