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

1.4.4 and 1.4.5 slower than 1.4.3 page with many nested popups #7839

Closed
matthieubrunet opened this Issue Nov 13, 2014 · 16 comments

Comments

Projects
None yet
5 participants
@matthieubrunet

matthieubrunet commented Nov 13, 2014

Issue description

I'm using JQM for a big web app (single paged), and I noticed that jQM 1.4.4 and 1.4.5 were significativity slower than 1.4.3 to display some big pages. So I tried to make a simple test case, and I discovered it was not the size of the page content itself, but the number of nested popup in it. My main page have 7 nested popup, and on some slow devices (iPad mini), it add 2-3 seconds to the loading time.
It concerns only the first load. When the page is loaded in the cache, it is loaded as fast in both versions.

Test page

Here is the jsbin tes page :
jQM 1.4.3 : http://jsbin.com/fosah/1/
jQM 1.4.5 : http://jsbin.com/biquwu/1/
As my code is very simplified, I had to put 200 popup's to make it noticeable in a regular browser.

Steps to reproduce

  1. Load the page.
  2. Click "run with js" to be sure the second page is not yet loaded
  3. Click on the "click here" button.
  4. Wait the page to load, and look at the number of ms displayed

Expected outcome

The two version should take the same time

Actual outcome

The 1.4.5 version is 1.5 - 2 time slower.
My test on Google chrome 38 and Safari 6.2
v 1.4.3 = arround 650ms
v 1.4.5 = around 1100ms

Platforms/browsers (including version) and devices tested

iMac : Chrome 38 and Safari 6.2
iPad Mini 16GO : iOS 7, Safari mobile

@Ruffio

This comment has been minimized.

Show comment
Hide comment
@Ruffio

Ruffio Nov 13, 2014

I can confirm the difference, but it is unusual to have 200 popups on the
same page. I know the 200 popups is for making the time noticable, but this
seems more like a synthetic test and perhaps the real issue is not shown in
this test. Would it be completely impossible to make testpages with the
'real' setup (nested popups) with (real) data?

I don't know your usecase but consider to use only one popup and the change
the content before loading it. That is how I do it.

Ruffio commented Nov 13, 2014

I can confirm the difference, but it is unusual to have 200 popups on the
same page. I know the 200 popups is for making the time noticable, but this
seems more like a synthetic test and perhaps the real issue is not shown in
this test. Would it be completely impossible to make testpages with the
'real' setup (nested popups) with (real) data?

I don't know your usecase but consider to use only one popup and the change
the content before loading it. That is how I do it.

@matthieubrunet

This comment has been minimized.

Show comment
Hide comment
@matthieubrunet

matthieubrunet Nov 13, 2014

Well, I filled an issue with the real data yesterday (#7837), but it was refused because the code was too complex :+)
So I worked hard to simplify the case.
As I said, my real app is only with 7 popups, and the difference is not noticeable on a regular browser, as you will see if you try those cases : (The links in my first issue are outdated)
v 1.4.3 : http://jsbin.com/nobome/1/
v 1.4.5 : http://jsbin.com/xozime/4/
But if you try it on iPad, you will see the difference (3700 vs 4700 on my iPad). And it's not a completely real case, as my application contain a lot of page, and a very big javascript file, that accentuate the difference.
Your advice about using the same popup is good, but it would be a lot of work for me, so right now, I prefer continue with 1.4.3.
But if you said me this performance loss will remain in future version, I'll be obliged to.
Thanks a lot your time. By the way, jQM is really great !

matthieubrunet commented Nov 13, 2014

Well, I filled an issue with the real data yesterday (#7837), but it was refused because the code was too complex :+)
So I worked hard to simplify the case.
As I said, my real app is only with 7 popups, and the difference is not noticeable on a regular browser, as you will see if you try those cases : (The links in my first issue are outdated)
v 1.4.3 : http://jsbin.com/nobome/1/
v 1.4.5 : http://jsbin.com/xozime/4/
But if you try it on iPad, you will see the difference (3700 vs 4700 on my iPad). And it's not a completely real case, as my application contain a lot of page, and a very big javascript file, that accentuate the difference.
Your advice about using the same popup is good, but it would be a lot of work for me, so right now, I prefer continue with 1.4.3.
But if you said me this performance loss will remain in future version, I'll be obliged to.
Thanks a lot your time. By the way, jQM is really great !

@Ruffio

This comment has been minimized.

Show comment
Hide comment
@Ruffio

Ruffio Nov 13, 2014

@matthieubrunet I don't know about future versions of jQM. I'm just a 'user/developer' like you, but I try to help the dev. team by testing as much as I can. Just to be clear.

Ruffio commented Nov 13, 2014

@matthieubrunet I don't know about future versions of jQM. I'm just a 'user/developer' like you, but I try to help the dev. team by testing as much as I can. Just to be clear.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Nov 13, 2014

Member

@matthieubrunet @Ruffio We take performance regressions very seriously and will look into this despite the case being a bit convoluted. ( the original case had an incredible amount of widgets on the page as well just not all popups ) This is generally a bad design and UI choice especially on mobile devices. I suggest you try to think of ways to simplify your design and look at it from a users perspective rarely is a page with more then a dozen widgets on it good for user experience.

All that said convoluted or not this seems a serious enough performance regression that its worth looking into.

Note changing versions in the bins shows the regression is actually from 1.4.3 to 1.4.4 so i'm going to change the title of this issue. Looking at the changes here I don't see anything obvious that would cause this so we will have to do some investigating.

Member

arschmitz commented Nov 13, 2014

@matthieubrunet @Ruffio We take performance regressions very seriously and will look into this despite the case being a bit convoluted. ( the original case had an incredible amount of widgets on the page as well just not all popups ) This is generally a bad design and UI choice especially on mobile devices. I suggest you try to think of ways to simplify your design and look at it from a users perspective rarely is a page with more then a dozen widgets on it good for user experience.

All that said convoluted or not this seems a serious enough performance regression that its worth looking into.

Note changing versions in the bins shows the regression is actually from 1.4.3 to 1.4.4 so i'm going to change the title of this issue. Looking at the changes here I don't see anything obvious that would cause this so we will have to do some investigating.

@matthieubrunet

This comment has been minimized.

Show comment
Hide comment
@matthieubrunet

matthieubrunet Nov 13, 2014

@arschmitz Thanks a lot for you message.
I agree with you that there is too much widget on this page, but my client is specific about being able to do everything without changing page. You should see the amount of widget I successfully refused to add… ;+)

@Ruffio Thanks a lot for your time, it's always helpful

matthieubrunet commented Nov 13, 2014

@arschmitz Thanks a lot for you message.
I agree with you that there is too much widget on this page, but my client is specific about being able to do everything without changing page. You should see the amount of widget I successfully refused to add… ;+)

@Ruffio Thanks a lot for your time, it's always helpful

@jaspermdegroot jaspermdegroot added this to the 1.5.0 milestone Nov 13, 2014

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Nov 13, 2014

Member

@arschmitz

I did a git bisect with the original test page (with the 200 popups with headers) and it looks like this commit introduced the performance regression: 98ece81. Do you see anything in there that could have caused did?

Member

jaspermdegroot commented Nov 13, 2014

@arschmitz

I did a git bisect with the original test page (with the 200 popups with headers) and it looks like this commit introduced the performance regression: 98ece81. Do you see anything in there that could have caused did?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Nov 13, 2014

Member

@jaspermdegroot ok i see whats happening. Each of the popups has a toolbar. The toolbar logic has become substantially more complicated to deal with the internal / external toolbar combos possible. We will need to look at how we can improve this. I guess the short term solution don't use so many toolbars :)

Member

arschmitz commented Nov 13, 2014

@jaspermdegroot ok i see whats happening. Each of the popups has a toolbar. The toolbar logic has become substantially more complicated to deal with the internal / external toolbar combos possible. We will need to look at how we can improve this. I guess the short term solution don't use so many toolbars :)

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Nov 13, 2014

Member

@matthieubrunet remove the data-role="header" from your headers in the toolbars and just add the classes directly to fix this.

Member

arschmitz commented Nov 13, 2014

@matthieubrunet remove the data-role="header" from your headers in the toolbars and just add the classes directly to fix this.

@matthieubrunet

This comment has been minimized.

Show comment
Hide comment
@matthieubrunet

matthieubrunet Nov 14, 2014

@arschmitz thanks a lot, it worked ! I've still a small difference on my real case (3,5s instead of 3s), so I suppose there is something else. If I found the origin, I'll post a new issue.

matthieubrunet commented Nov 14, 2014

@arschmitz thanks a lot, it worked ! I've still a small difference on my real case (3,5s instead of 3s), so I suppose there is something else. If I found the origin, I'll post a new issue.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 29, 2014

Contributor

@arschmitz In fixedToolbar.js we should not unconditionally initialize this.pagecontainer, because that will initialize it for fixed an non-fixed toolbars alike. It seems that $( ":mobile-pagecontainer" ) is a really expensive selector, so we should restrict it to fixed toolbar initialization. I'll make a PR to that effect.

Contributor

gabrielschulhof commented Nov 29, 2014

@arschmitz In fixedToolbar.js we should not unconditionally initialize this.pagecontainer, because that will initialize it for fixed an non-fixed toolbars alike. It seems that $( ":mobile-pagecontainer" ) is a really expensive selector, so we should restrict it to fixed toolbar initialization. I'll make a PR to that effect.

gabrielschulhof added a commit to gabrielschulhof/gabrielschulhof.github.com that referenced this issue Nov 29, 2014

gabrielschulhof added a commit to gabrielschulhof/gabrielschulhof.github.com that referenced this issue Nov 29, 2014

gabrielschulhof added a commit to gabrielschulhof/gabrielschulhof.github.com that referenced this issue Nov 29, 2014

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 29, 2014

Contributor

@arschmitz Also, we shouldn't use $( ":mobile-pagecontainer" ), because that traverses the entire DOM, and we know that the page container must be a parent of this.element for it to be interesting, so we should use this.element.closest( ":mobile-pagecontainer" ).

Contributor

gabrielschulhof commented Nov 29, 2014

@arschmitz Also, we shouldn't use $( ":mobile-pagecontainer" ), because that traverses the entire DOM, and we know that the page container must be a parent of this.element for it to be interesting, so we should use this.element.closest( ":mobile-pagecontainer" ).

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 29, 2014

Contributor

Foregoing the construction of $( ":mobile-pagecontainer" ) during toolbar instantiation speeds up toolbar creation by a factor of 2: http://jsperf.com/jqm-7839-fixed-toolbar

Contributor

gabrielschulhof commented Nov 29, 2014

Foregoing the construction of $( ":mobile-pagecontainer" ) during toolbar instantiation speeds up toolbar creation by a factor of 2: http://jsperf.com/jqm-7839-fixed-toolbar

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 29, 2014

Contributor

More on Chrome, but (my) Chrome for some reason fails to POST to browserscope.

Contributor

gabrielschulhof commented Nov 29, 2014

More on Chrome, but (my) Chrome for some reason fails to POST to browserscope.

@gabrielschulhof gabrielschulhof self-assigned this Nov 29, 2014

@Ruffio

This comment has been minimized.

Show comment
Hide comment
@Ruffio

Ruffio Nov 29, 2014

I can confirm the diff of factor 2 in Chrome on Nexus 7.

Would selector optimization elsewhere in the code be relevant too in order
to speed up the code?

Ruffio commented Nov 29, 2014

I can confirm the diff of factor 2 in Chrome on Nexus 7.

Would selector optimization elsewhere in the code be relevant too in order
to speed up the code?

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Nov 30, 2014

Member

@Ruffio yes we should ensure we are not using pseudo selectors else where if it can be avoided

Member

arschmitz commented Nov 30, 2014

@Ruffio yes we should ensure we are not using pseudo selectors else where if it can be avoided

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Dec 1, 2014

Contributor

@cgack is this.pagecontainer needed for all fixed toolbars or only external fixed toolbars?

Contributor

gabrielschulhof commented Dec 1, 2014

@cgack is this.pagecontainer needed for all fixed toolbars or only external fixed toolbars?

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