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

Convert admin/content to a View #150

Closed
quicksketch opened this issue Jan 5, 2014 · 16 comments · Fixed by backdrop/backdrop#573
Closed

Convert admin/content to a View #150

quicksketch opened this issue Jan 5, 2014 · 16 comments · Fixed by backdrop/backdrop#573

Comments

@quicksketch
Copy link
Member

In the Drupal 8 issue, admin/content apparently provides a non-Views fallback if Views module is not enabled. For Backdrop, that's a probably unnecessary level support. The listing should be provided by Views and if you don't have it enabled, the listing isn't there.

Relevant drupal.org issue: https://drupal.org/node/1895160

This issue will be dependent upon #142.

@DirectorHaas
Copy link

Willing to take a look. Questions:

  1. Are we ready for this, is views in core good to start on creating these views?
  2. Does D8 work already have some importable/compatible views, or is it recommended to create views/views-code from scratch.
    Thanks!

@quicksketch
Copy link
Member Author

Hey @QuantumTime, answers:

  1. Are we ready for this, is views in core good to start on creating these views?

Views in core is ready to start creating views, but we don't have Views Bulk Operations completed yet (#142). So for now we can't create views that need actions/operations on them.

  1. Does D8 work already have some importable/compatible views, or is it recommended to create views/views-code from scratch.

D8 is a good reference, but for construction of our views, we'll probably reconfigure them from scratch, since I believe several of the configuration options of views were renamed in D8.

For starters, I think #162 is going to be the easiest (/node).

@dboulet
Copy link
Member

dboulet commented Feb 15, 2014

Great work so far on getting Views in core, quicksketch. I apologize if this has been discussed elsewhere, but has any research been done to explore the potential performance hit of converting these hard-coded listings to views? I know that performance is a major priority for this project and the additional overhead of loading views on these pages should be a consideration. Pages like admin/content and admin/people are some of the most-visited administration pages and are not good candidates for caching—perhaps keeping hard-coded fallbacks for these would be a good idea.

@quicksketch
Copy link
Member Author

Hi @dboulet, thanks for joining in here! Views definitely does have some performance impact. I haven't benchmarked the admin/content or admin/people pages, but I did benchmark the /node page several months ago in a D7 vs. D7 Views vs. D8 Views. The results of those benchmarks are here: https://docs.google.com/spreadsheet/ccc?key=0ArRjqXXmdU26dGc2YThZVFQyRnZodlFtdkxKeW4xRmc&usp=drive_web#gid=3

There's a significant slowdown for /node in D7 vs. D7 Views, 25% actually. I imagine other views will be similar vs. manually building the page. However, I don't think that's a serious problem for admin pages. In these benchmarks, /node went from 116ms to 153ms. While 25% sounds bad, a difference of 37ms isn't going to be noticeable and is going to be worth it for the flexibility that Views offers.

@dboulet
Copy link
Member

dboulet commented Feb 16, 2014

Thanks @quicksketch, I definitely agree with you that providing the listings as views is worth it—but for those who will leave the defaults untouched and don’t need the flexibility that views offers, the performance trade-off is unnecessary.

You’re right that a few dozen milliseconds is not much, but the difference could be much greater on cheap shared hosting, for example. Plus, it only takes a few small “unnoticeable” changes like this to add up to a very noticeable one. We’ve all seen the studies where large sites purposely increased page load time by only small fraction of a second and saw a real drop in user engagement—this stuff matters. A snappy user interface which appears to load instantaneously is one that is a pleasure to use—meaning the page should ideally load within 0.1 seconds.

I’d like to see Backdrop take these kinds of issues very seriously. I see this as another way to differentiate the project from Drupal—which has constantly favoured flexibility and extensibility at the expense of performance.

Perhaps in this case we could consider removing the hard-coded administration listings only after having conducted performance tests against those provided by views. If there is indeed not a large difference in load times, then by all means keep it views-only.

@Gormartsen
Copy link
Member

I really worry about going to views from a performance perspective.

Check this profiling output here: http://pastebin.com/WRV7zYx2

It is output of site/frontpage with one node. Cache enabled.

This one from site/node : http://pastebin.com/1TW7aEeJ

I will boil it down for you:

::node_page_default() [1.08 Mb] [realtime: 15.834 ms]
::views_page() [3.24 Mb] [realtime: 30.224 ms]

Kinda almost 2 times slower with views (cache enabled).

@Gormartsen
Copy link
Member

We more things.

frontpage : http://pastebin.com/xFQs9yw3
node: http://pastebin.com/tRuRzQpa

Views generated +4k calls at this case.

@quicksketch
Copy link
Member Author

@Gormartsen thanks for your input. If you're concerned mostly about the /node homepage, it might be better to post in #144, which is for replacing the homepage. For something like an administration page that gets little traffic, a 10-20ms difference probably won't be significant. In this case, switching to a View also gets us some new functionality (namely search) that would be super handy for administration, plus of course the ability of views to customize it.

@quicksketch
Copy link
Member Author

For admin/content/node, I've filed a PR at backdrop/backdrop#573. This addresses a couple of things since this is the first time we've used views bulk operations in action.

  • Converted admin/content/node to a view, all test pass as-is with just adjustments for form element names. And added an upgrade path to create this view for Drupal 7 upgraders.
  • It updates the VBO form on the view to operate more like the Drupal 7 contrib module, placing actions in a fieldset to separate it from the filter form. Previously all fields were just floating right next to each other, so you couldn't tell where filtering stopped and actions begun.
  • It fixes Views' ability to use responsive tables, to match the responsive tables that were previously at the same path.
  • Views was hiding the field to set menu item weight unless the menu item was a local task. Since admin/content needs to be -10, I couldn't set this value. This is now fixed and weight can be set when a menu item is a tab or a normal item.
  • Styled the VBO operations form and the exposed filter form to be consistent with each other.

@quicksketch
Copy link
Member Author

Here are the performance numbers for this conversion. With converting admin/content/node into a view, with identical configuration of fields, responsive columns, and dropbuttons. I made minor modifications to remove sticky table headers (since columns are obvious from their content) and to add a title search exposed filter.

Overall XHProf numbers:

views-node-bulk

So pretty bad. Views adds almost twice as many function calls (92% more) plus takes 75% more time and 50% more memory.

Digging into the XHProf numbers, I found that the bulk of the cost here was from field-level theming. I filed two issues #447 (merged at the time of these benchmarks) and #448 (not yet merged) to help alleviate per-field rendering problems.

Despite these problems, I don't think not using Views is really an option. With 80% of sites using Views by Drupal.org usage statistics, most sites out there are using Views for one purpose or another. Converting administrative listing seems like the most ideal conversions considering it adds flexibility and new functionality while affecting the smallest number of users in a limited way. Although this is an increase from 360ms to 640ms (on my machine), it's not a number that would get proportionally worse based on site expansion or outside functionality (unlike a front-end listing).

Although increasing the performance of Views is going to be difficult (it's what I've been looking at for the last day) we should focus our efforts on improving it rather than supporting non-Views fallbacks. People are going to use Views on their sites; it's completely inevitable within the market we're serving. The best we can do is provide optimized views out-of-box and make Views itself more efficient where possible.

@ghost
Copy link

ghost commented Dec 11, 2014

That'll teach me to assign issues to myself in future once I start working on them - I started this a few days ago but hadn't had time to come back to it. At least @quicksketch was able to do it and find/fix those VBO bugs 😄

@ar-jan
Copy link

ar-jan commented Dec 11, 2014

Could the approach taken by Views Accelerator be used in some way to improve performance?

Views Accelerator improves post-execution times by swapping out the post_execute() function of views fields for a high-performant, low-rendering version.

@quicksketch
Copy link
Member Author

Doh, sorry @BWPanda. I didn't mark it assigned either. :(

This provides a good example of how to make an upgrade path. So far I think the hold-up on all the other convert-to-views issues is that we don't have an update hook.

Could the approach taken by Views Accelerator be used in some way to improve performance?

I've been looking into similar solutions (hence the two new issues at #447 and #448), but Views Accelerator primarily is for accelerating Field module fields:

Q: What kind of views and fields can be accelerated?
A: Any View that contains Fields, as in fields from the Fields API, the ones you add to your user and content types via the "Manage Fields" tab. The module does not affect basic properties, like content title, content creation date -- those are already snappy.

This view (and other administrative views) won't have any Field API fields in listings by default, so there won't be anything to accelerate. In general, I think we should be looking at how to optimize the rendering of fields.

I also tried using the render cache, which works beautifully (cut second-load time down more than half), but it would cause the operation links for "edit" and "delete" to display to all users equally, regardless of permission. Since it's possible to have administrator access this page but not be able to edit/delete all content (such as on a site with access control modules).

@quicksketch
Copy link
Member Author

Here are updated numbers after merging in #448.

xhprof-views-node

Still very much slower, but at least it's slightly less slower (6% improvement) than before. I still don't think these depressing numbers should prevent our adoption of Views within administrative listings. I think we should knowingly accept them and continue to optimize Views to close the gap between the traditional listings and Views as much as possible.

@klonos
Copy link
Member

klonos commented Dec 15, 2014

I don't mind the slow down when it brings so many benefits. Views everywhere +++

@quicksketch
Copy link
Member Author

Agreed, the decision to use Views was made by the users of Drupal quite some time ago by popular demand. I've merged in the PR at backdrop/backdrop#573. Let's use the pattern to continue our other conversions ASAP.

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

Successfully merging a pull request may close this issue.

7 participants