Profiling a homebox page I am seeing over 250ms spent in _homebox_check_views_block_access().

homebox executes hook_block() - and views_block() itself checks the access of the view before returning it.

_homebox_check_views_block_access() code duplicates all that code again, as far as I can see the access check will always return true, because the access was already checked by views in the first place.

So this patch removes the function entirely.

CommentFileSizeAuthor
#1 Desk 1_003.png32.09 KBcatch
views_access.patch1.77 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Performance
StatusFileSize
new Desk 1_003.png32.09 KB

Here's a screenshot from webgrind showing the time spent.

drumm’s picture

drumm
he/him
Location NY, US
commented

The only caveat I can think of is caching. But if a view can only be accessed by role, it had better be BLOCK_CACHE_PER_ROLE or other suitable configuration.

catch’s picture

catch
he/him
Primary language English
commented

View access checks happen outside the caching (which is usually by query I think) - to get something from views cache you have to instantiate the view, and access etc. is all available there - caching only happens at the query execution and rendering points which are much later iirc.

drumm’s picture

drumm
he/him
Location NY, US
commented

Homebox does a bit of block caching, like the core block module does. I assume views works with this, but haven't confirmed.

catch’s picture

catch
he/him
Primary language English
commented

I'll have a look at that tomorrow to see how it interacts. Views specifies CACHE_CUSTOM (or whatever no caching is) for its blocks, then caches internally, so if it respects what comes from block_list() should be OK.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Reviewed & tested by the community

Then I believe this is good. However, I have not actually run this code. (I was always wondering why this check existed, but never looked into it.)

catch’s picture

catch
he/him
Primary language English
commented

Alright had a look through the homebox and views code.

homebox is respecting block cache modes.

Views uses BLOCK_NO_CACHE by default.

If in views admin, you specify that it should use block caching, then you can use per-role, per-page, per-user etc. - in that case views is handing off the caching to core block module (and not doing an access check) so it seems fine if homebox works the same for this.

I also have not run this code but it should hopefully get some testing this week.

brianV’s picture

brianV commented

Just updating this:

I never really understood the logic of this snippet either, but I didn't want to remove it before looking into it further, which it looks like you just did. Certainly, your logic seems sound.

Catch: did you have a chance to test this code, as your last post indicated you might?

brianV’s picture

brianV commented
Status: Reviewed & tested by the community » Fixed

Did some brief checking, then pushed to -dev for both D6 and D7.

Automatically closed -- issue fixed for 2 weeks with no activity.