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

Performance: forced evaluation of camelCase(key) in $.fn.data() #1941

Closed
victor-homyakov opened this Issue Dec 17, 2014 · 22 comments

Comments

Projects
None yet
4 participants
@victor-homyakov
Contributor

victor-homyakov commented Dec 17, 2014

When retrieving data from jQuery element with $element.data(key), jQuery needlessly calculates camel-cased version of key jQuery.camelCase( key ) at https://github.com/jquery/jquery/blob/master/src/data.js#L116 even when data could be accessed via original key (or it is already camel-cased).

This code could be rewritten in order to evaluate camel-cased key only when data_user.get( elem, key ) has failed or when user is setting the data.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 17, 2014

Member

Would you like to create a pull request? If it can be easily skipped we might as well.

Member

dmethvin commented Dec 17, 2014

Would you like to create a pull request? If it can be easily skipped we might as well.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 17, 2014

Member

Would be good to see a jsperf on this as well.

Member

dmethvin commented Dec 17, 2014

Would be good to see a jsperf on this as well.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 17, 2014

Member

Pretty sure that diff will be negligible or even worsens the code-path, since you would have add check for it which reflect on the performance.

/cc @rwaldron

Member

markelog commented Dec 17, 2014

Pretty sure that diff will be negligible or even worsens the code-path, since you would have add check for it which reflect on the performance.

/cc @rwaldron

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Dec 18, 2014

Member

I'm certain this will just break tests for no reason. camelKey is used inside an if consequent that returns at the end (regardless of outcome), and in the alternate path. If the assignment was moved into the first condition, it would have to be duplicated in the alternate path.

I appreciate that devs want to tweak and tinker, but this case isn't going to benefit jQuery

Member

rwaldron commented Dec 18, 2014

I'm certain this will just break tests for no reason. camelKey is used inside an if consequent that returns at the end (regardless of outcome), and in the alternate path. If the assignment was moved into the first condition, it would have to be duplicated in the alternate path.

I appreciate that devs want to tweak and tinker, but this case isn't going to benefit jQuery

@rwaldron rwaldron closed this Dec 18, 2014

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov
Contributor

victor-homyakov commented Dec 18, 2014

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

As you can see, retrieving data when key is already camel-cased could be made 1.5..2 times faster in Chrome and Firefox (and without any penalty for other cases).

The only cost, as @rwaldron already said, is camelKey = jQuery.camelCase( key ); repeated twice.

Contributor

victor-homyakov commented Dec 18, 2014

As you can see, retrieving data when key is already camel-cased could be made 1.5..2 times faster in Chrome and Firefox (and without any penalty for other cases).

The only cost, as @rwaldron already said, is camelKey = jQuery.camelCase( key ); repeated twice.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 18, 2014

Member

Did it break a test-suite?

Member

markelog commented Dec 18, 2014

Did it break a test-suite?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

No

Contributor

victor-homyakov commented Dec 18, 2014

No

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 18, 2014

Member

weeeeird, @rwaldron seems we should re-open for further investigation, need to look at the perf tests, implementation and byte-size hit.

Member

markelog commented Dec 18, 2014

weeeeird, @rwaldron seems we should re-open for further investigation, need to look at the perf tests, implementation and byte-size hit.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Dec 18, 2014

Member

@markelog

Did it break a test-suite?

It wouldn't have, as long as this is true:

The only cost, as @rwaldron already said, is camelKey = jQuery.camelCase( key ); repeated twice.

Which it appears to be so.

Member

rwaldron commented Dec 18, 2014

@markelog

Did it break a test-suite?

It wouldn't have, as long as this is true:

The only cost, as @rwaldron already said, is camelKey = jQuery.camelCase( key ); repeated twice.

Which it appears to be so.

@rwaldron rwaldron reopened this Dec 18, 2014

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 18, 2014

Member

@victor-homyakov could you send us a PR?

Member

markelog commented Dec 18, 2014

@victor-homyakov could you send us a PR?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Yes. As far as I understand, no additional tests needed?

Contributor

victor-homyakov commented Dec 18, 2014

Yes. As far as I understand, no additional tests needed?

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Dec 18, 2014

Member

Is there a patch?

Member

rwaldron commented Dec 18, 2014

Is there a patch?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Patch? I fear I don't understand what you mean.

Contributor

victor-homyakov commented Dec 18, 2014

Patch? I fear I don't understand what you mean.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Dec 18, 2014

Member

Yes, can you submit code changes you've made as a Pull Request?

Member

rwaldron commented Dec 18, 2014

Yes, can you submit code changes you've made as a Pull Request?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Yes, that's what I'm doing right now. Also could you please guide me how to write commit message properly according to style of this repository?

Contributor

victor-homyakov commented Dec 18, 2014

Yes, that's what I'm doing right now. Also could you please guide me how to write commit message properly according to style of this repository?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Something like "Core: speed up $.fn.data() for camel-cased key"?

Contributor

victor-homyakov commented Dec 18, 2014

Something like "Core: speed up $.fn.data() for camel-cased key"?

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Dec 18, 2014

Member

Data: speed up data access for camel-cased keys

Member

rwaldron commented Dec 18, 2014

Data: speed up data access for camel-cased keys

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Is any additional description needed? Or maybe link to jsperf test?

Contributor

victor-homyakov commented Dec 18, 2014

Is any additional description needed? Or maybe link to jsperf test?

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Dec 18, 2014

Contributor

Also

Running "compare_size:files" (compare_size) task
   raw     gz Sizes
246927  73198 dist/jquery.js
 84310  29385 dist/jquery.min.js

   raw     gz Compared to master @ d6c97abb744bfe8c67ab9158aecdb5bb1c05e47b
   +53    +13 dist/jquery.js
   +19     -1 dist/jquery.min.js

   raw     gz Compared to last run
   +53    +13 dist/jquery.js
   +19     -1 dist/jquery.min.js
Contributor

victor-homyakov commented Dec 18, 2014

Also

Running "compare_size:files" (compare_size) task
   raw     gz Sizes
246927  73198 dist/jquery.js
 84310  29385 dist/jquery.min.js

   raw     gz Compared to master @ d6c97abb744bfe8c67ab9158aecdb5bb1c05e47b
   +53    +13 dist/jquery.js
   +19     -1 dist/jquery.min.js

   raw     gz Compared to last run
   +53    +13 dist/jquery.js
   +19     -1 dist/jquery.min.js
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 18, 2014

Member

Don't worry about commit message we adjust it if needed

Member

markelog commented Dec 18, 2014

Don't worry about commit message we adjust it if needed

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov
Contributor

victor-homyakov commented Dec 18, 2014

@markelog markelog closed this in 72c4a06 Dec 23, 2014

markelog added a commit that referenced this issue Nov 10, 2015

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018

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