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

Create performance tests #2267

Closed
jamesplease opened this issue Feb 4, 2015 · 10 comments
Closed

Create performance tests #2267

jamesplease opened this issue Feb 4, 2015 · 10 comments

Comments

@jamesplease
Copy link
Member

Once #2253 lands, people are concerned about the perf of the new ItemView with Regions. This may be a problem with the current implementation of ItemView, but it shouldn't be hard to optimize.

  1. Create perf tests comparing CollectionView with old ItemView vs. CollectionView with new ItemView
  2. Make any changes as necessary

The one thing that they've brought up is the eager instantation of Regions (#2109). We need to create two lifecycle paths for our ItemViews to reflect the two sorts of views. The first type is a view that is instantiated with an existing element. It may have HTML that already exists on the page. It makes sense for this view to instantiate its regions at creation time. The other type of view – by far the most common – is one that does not have an element at creation time, and will not have any HTML until render is called. ItemView treats these two cases exactly the same, but in the latter case we can hold off instantiating the regions until later on.

There might be some other consequences to this change that I haven't thought of. Do point them out if you can think of any!

If we go ahead and make this change, though, then we should see basically 0 perf difference from the switch.

@jamiebuilds
Copy link
Member

We're not going to see any significant perf boosts for our view layer without making some major changes. Let's get 3.0 out and focus on that for 4.0, I have a ton of ideas.

@jamesplease
Copy link
Member Author

I agree there's more we can do going forward, but if rendering a list of LayoutViews is 80% slower than rendering a list of ItemViews, then we will need to make changes in v3. This is more about ensuring that our CollectionView isn't insanely slow because of merging LayoutView into ItemView.

@jamiebuilds
Copy link
Member

I've looked into this previously. There's very little difference between ItemView and a LayoutView without any added regions. I honestly don't get this concern that it's suddenly ridiculously slow.

@megawac
Copy link
Member

megawac commented Feb 4, 2015

@thejameskyle were'nt you concerned about the cases where people had things like

Marionette.ItemView.extend({
    modelEvents: {
       "change": "render"
    },
    regions: {

     },
    template: "#-.-#"
})

Rerendering the regions over and over

@jamesplease
Copy link
Member Author

I honestly don't get this concern that it's suddenly ridiculously slow.

It's a concern because it's a possibility. The actionable item that will close this issue is when we have perf tests that show that the new CollectionView is just as performant as the last one in a variety of situations, like the one @megawac showed above.

@jamiebuilds
Copy link
Member

@megawac I haven't said anything about that case but then that's just coming down to the speed of _.invoke which is presumably fast with an empty object.

The only time someone would experience a "slowdown" is in the constructor where it initializes the region manager. This will be solved by switching to the mixin we spoke about.

In the future I'm all in favor of making regions faster (aka. replacing them with something better), but today we can continue with the decent-enough perf we've always had.

@jasonLaster
Copy link
Member

@jmeas - i'm a big fan of perf coverage. I'd like to see this done separately though and not tied to a release. and not tied to a refactor of ItemView.

@jamesplease
Copy link
Member Author

I'd like to see this done separately though and not tied to a release. and not tied to a refactor of ItemView.

I agree that there are perf changes we can hold off on and do separately. But there are others that we cannot hold off on, and that's what this issue is about. If we make a drastic change to the Marionette, such that the CollectionView could conceivably be made slower, then we owe it to our users to verify that it isn't drastically slower.

Imagine if we released this and CollectionView was somehow 80% slower. We couldn't be like, "Oh, we didn't want performance to hold back the release" Nawm sayin? Of course, it's unlikely that it will be 80% slower, but if it was somehow 10-20% slower in some situations, then I think that's worth looking into.

All that I'm saying is...let's look at the performance before releasing v3. People use CollectionViews to render thousands of items. We don't want to screw them over.

@samccone
Copy link
Member

samccone commented Feb 4, 2015

yeah, I would like to get some perf specs inside of marionette 2.x so at least we have an idea of if we did something bad, otherwise we are flying blind.

@ahumphreys87 ahumphreys87 modified the milestone: v3.0.0 Apr 12, 2015
@ahumphreys87 ahumphreys87 changed the title Optimize ItemView Create performance tests Apr 12, 2015
@paulfalgout
Copy link
Member

#3182 seems like it is moving along.. going to close this.. but we can reopen if that issue doesn't resolve.

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

No branches or pull requests

7 participants