Performance updates for virtual_attribute_search #557
Conversation
|
@lpichler Can I have you take a look at this and make sure this still falls in line with the goals of ManageIQ/manageiq#15145 ? I might adds some tests before calling this good, but would at least like your opinion if what I changed here seems kosher. Thanks! |
@NickLaMuro Looks good to me, I tested out - nice finding - just question to - would be helpful to bring your thanks! |
|
@lpichler Thanks for taking a look!
Oh, I assume you mean this part: return resource_attr unless Rbac::Filterer.new.send(:apply_rbac_directly?, klass)Then yeah, I think this makes sense to do. I can add that and hopefully getting to writing some tests for this today as well. Thanks again! |
|
Note: This is for speeding up https://bugzilla.redhat.com/show_bug.cgi?id=1656371 |
|
|
:)
|
@NickLaMuro does it make sense to add this bail out shortcut to |
NickLaMuro
force-pushed
the
optimized_virtual_attribute_search
branch
from
07178e7
to
c1d7f9e
Compare
NickLaMuro
changed the title
NickLaMuro
force-pushed
the
optimized_virtual_attribute_search
branch
from
c1d7f9e
to
ccd826b
Compare
|
Added some specs on this one, so removed the |
Previously, the check against the `resource_attr` was only handling
cases where the resource was "ActiveRecord-like", but didn't properly
handle when the record in question wasn't something that we apply Rbac
on to begin with.
This properly makes that check (in a slightly hacky fashion).
Benchmarks:
-----------
Example request:
```
url: /api/services
attributes:
expand: resources
attributes:
- picture
- picture.image_href
- chargeback_report
- evm_owner.userid
- v_total_vms
- power_state
- all_service_children
- tags
filter[]: ancestry=null
sort_by: created_at
sort_options: nil
sort_order: desc
limit: 20
offset: 0
```
**Before**
```
| ms | queries | query (ms) | rows |
| ---: | ---: | ---: | ---: |
| 2197 | 322 | 194.4 | 1627 |
| 953 | 321 | 182.7 | 196 |
| 996 | 321 | 187.1 | 196 |
```
**After**
```
| ms | queries | query (ms) | rows |
| ---: | ---: | ---: | ---: |
| 2274 | 282 | 166.2 | 1612 |
| 858 | 281 | 147.9 | 181 |
| 960 | 281 | 164.2 | 181 |
```
Previously, the same record that was already "Rbac'd" in the initial
`.collection_search` for an `index` action would be re-done for each
`virtual_attribute` that wasn't in a `has_many`-like relationship.
This made little sense as it wasn't provided any better granularity for
permissions, since the permissions where already checked once before,
and it was a pretty obvious `N+1`. Also checks that the class is
filterable by Rbac, otherwise that will also just return the attribute.
Benchmarks:
-----------
Example request:
```
url: /api/services
attributes:
expand: resources
attributes:
- picture
- picture.image_href
- chargeback_report
- evm_owner.userid
- v_total_vms
- power_state
- all_service_children
- tags
filter[]: ancestry=null
sort_by: created_at
sort_options: nil
sort_order: desc
limit: 20
offset: 0
```
**Before** (previous commit)
```
| ms | queries | query (ms) | rows |
| ---: | ---: | ---: | ---: |
| 2274 | 282 | 166.2 | 1612 |
| 858 | 281 | 147.9 | 181 |
| 960 | 281 | 164.2 | 181 |
```
**After**
```
| ms | queries | query (ms) | rows |
| ---: | ---: | ---: | ---: |
| 2004 | 263 | 156.8 | 1593 |
| 756 | 261 | 142.9 | 161 |
| 769 | 261 | 138.9 | 161 |
```
Note: Metrics were taken three times right after a server restart, so
the first request suffers from autoloading slowness.
NickLaMuro
force-pushed
the
optimized_virtual_attribute_search
branch
from
ccd826b
to
1ad20d2
Compare
|
Checked commits NickLaMuro/manageiq-api@9cedec7~...1ad20d2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@miq-bot assign @abellotti |
|
@abellotti does this look good for you? |
|
@miq-bot add_label bug |
|
This looks great @NickLaMuro Thanks for fixing this. 👍 |
Fixes two simple performance issues:
collection_search(second commit)Benchmarks
Before
After (first commit)
After (second commit)
TODO
Links