Performance: replace several jqmData() function calls with getAttribute() to improve performance. #5466

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

Changsuk commented Jan 17, 2013

Hi, All.
Actually, I tried to do a PR last week but I couldn't because there were many issues on Tizen project, so I'm sorry that I couldn't have enough time to do it. I'm opening the third PR regarding performance improvement now.

My team found another performance issue about using jqmData() function.
As you know, jqmData() calls jQuery data() function. "data()" function is complicated and not small. Also it takes time to call the function because many cases call jQuery.extend() function in data() and extend() calls other functions and has for loop and does a lot of things. Hence, my team found several jqmData() function calls which can be replaced with getAttribute() JavaScript(JS) function. (Some cases can not do like that.)
After replacing jqmData() function calls with getAttribute(), my team checked Web App.'s launching time and got a big performance improvement. Web App.'s average launching time decreased to 130ms on GalaxyS2 class devices.
My team has been looking for other codes which are able to replace jqmData() function call since then and if my team finds other codes to replace, I'll do a PR again.

Additionally, some codes using jqmData() return number or boolean value, hence some of committed codes on jquery.mobile.buttonMarkup.js are modified according to the return value's type.

  • Tests : The code has been tested on several jQM widgets using qunit.js.
  • Modified code is followed the jQuery Core Style guide.
  • Scope : Performance improvement.

If there are any problems or mistakes on that PR, please let me know. Thanks in advance.

Contributor

jerone commented Jan 17, 2013

Should the false result of an getAttribute method result in an undefined?
What is jqmData doing in those cases when the attribute is not found?
Shouldn't those result in an empty string?

You could make this into an internal function.

Contributor

gabrielschulhof commented Jan 17, 2013

.jqmData() actually calls .data() after adding the $.mobile.ns namespace to the key.

I'm not sure whether the replacements provided in this PR are equivalent to .jqmData(), because they merely set attributes on the DOM element, whereas .jqmData() also sets jQuery data items. Thus, is the corresponding retrieval is made using .jqmData() rather than .attr(), then the data will not get retrieved correctly.

@Changsuk, did the unit tests complete successfully with this change in place?

Owner

arschmitz commented Jan 17, 2013

@Changsuk in addition to @gabrielschulhof concerns above, while it is true using getAttribute may be faster getAttribute is not safe for cross browser use it does not always work properly in IE.

Also we use jquery's data functions to prevent potential problems with with circular references and memory leaks. data ensures all data is properly set and retrieved. it also ensure all data is removed when an element is removed from the dom.

Thank your for your PR though we really appreciate all the great suggestions for improving performance.

Contributor

Changsuk commented Jan 18, 2013

@jerone,
Should the false result of an getAttribute method result in an undefined?
-> Yes, If jqmData() can't find matched attribute or a data in an array, jqmData() returns undefined. The committed codes with undefined can meet the jqmData()'s return value.

What is jqmData doing in those cases when the attribute is not found?
-> As you may know, jqmData() calls jQuery.fn.extend.data() hence, if jQuery.fn.extend.data() doesn't find the attribute, it returns undefined. You can understand it with the jQuery codes and my comments below.

return data === undefined && parts[1] ? // If jQuery.data() doesn't find the attribute, the data is undefined.
this.data( parts[0] ) :
data; // It returns undefined.

Shouldn't those result in an empty string?
-> As you know, empty string and undefined are different. jQuery.fn.extend.data() returns undefined so I think that we should match the return value.

Contributor

Changsuk commented Jan 18, 2013

@gabrielschulhof,
Yes, my team did all of the unit test twice and all test cases are passed. Also my team verified all of jQuery Mobile widgets and Tizen widgets apparently. :)

@gabrielschulhof and @arschmitz,
As you can guess, the codes I modified doesn't need to store cache data on jQuery.cache because the cases are just retrieve the DOM elementary's attribute. Hence, my team thought that we could use JS API. As you can see the jQuery codes, jQuery.fn.extend.data() and jQuery.data() functions' call stacks are deep and do many things. Hence, the methods' call time are much longer. After my team adapted the codes on real devices using WebKit based browsers and analyzed launching time in detail with WebKit source code, we got a big improvement from the test.

@arschmitz,
My team check getAttribute() API on IE browser. It works OK. Also some of the modules in jQuery Mobile use setAttribute() JS API. I think that setAttribute() is the same case.

@ALL,
I should like to end by saying that, as you already know, jQuery Mobile is for mobile devices and performance issues are very important. So If you can, please consider that PR again. Thanks in advance.

Contributor

gabrielschulhof commented Jan 18, 2013

Another problem: jQuery's dataAttr() function, which is a jQuery internal function and which ultimately retrieves the value of the data-* attribute does a lot more than just retrieve the value. For example, it converts the value to boolean. Now, while we could do that work ourselves, we don't want to diverge too much from what jQuery does.

I did, however, achieve a performance boost by replacing the set call to .jqmData() inside mapToDataAttr() with $.removeData():http://jsperf.com/jqmdatavsremovedata/3

This basically has the effect that the data will not be cached, but it will always be gotten from/set on the DOM element.

Contributor

gabrielschulhof commented Jan 21, 2013

Well, it looks like we might have to look into doing things the native way, after all. The benefits are hard to overlook ...

Contributor

jerone commented Jan 21, 2013

On mobile every profit is a 'big' difference.
Just run that latest jspref on every browser on my device (8 atm :) ).

Contributor

gabrielschulhof commented Jan 22, 2013

Oops! There were other files in this PR as well. The above commit fixes only buttonMarkup. I'll reopen this so I don't forget the other files.

Contributor

Changsuk commented Jan 22, 2013

@ALL, Thanks for accepting my team's idea.
@gabrielschulhof I saw the test - http://jsperf.com/jqmdatavsremovedata/7. Thanks for your effort to make a new method and modify the other codes. After you modify the other files with new method - getAttrFixed, I'll do another PR to improve performance by using the method.

gabrielschulhof added a commit that referenced this pull request Feb 25, 2013

gabrielschulhof added a commit that referenced this pull request Feb 25, 2013

Contributor

gabrielschulhof commented Feb 25, 2013

I have now applied all the changes in this PR by hand.

Contributor

Changsuk commented Feb 28, 2013

@gabrielschulhof Thanks so much, Gabriel. I'll put another PR to complete replacing jqmData() to get/setAttribute. :)

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