Skip to content

Conversation

gaborigloi
Copy link
Contributor

Get rid of the unnecessary get_info function.

Signed-off-by: Gabor Igloi gabor.igloi@citrix.com

Get rid of the unnecessary get_info function.

Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 14.103% when pulling ffb7a84 on gaborigloi:xapi_vm_lifecycle_check_operation_error_cleanup into ab5bca8 on xapi-project:master.

@robhoes robhoes merged commit 1d90461 into xapi-project:master May 30, 2017
@gaborigloi gaborigloi deleted the xapi_vm_lifecycle_check_operation_error_cleanup branch June 8, 2017 13:20
let update_allowed_operations ~__context ~self =
let all, gm, clone_suspended_vm_enabled, vdis_reset_and_caching = get_info ~__context ~self in
let check accu op =
match check_operation_error __context all gm self clone_suspended_vm_enabled vdis_reset_and_caching op true with
Copy link
Contributor Author

@gaborigloi gaborigloi Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realised that maybe we did things this way for performance reasons: get the info from the db only once, and use that info when we loop through multiple VM operations here, to avoid doing the same DB accesses multiple times. With this change we do a Db.VM.get_record_internal, Db.VM_guest_metrics.get_record_internal, and a Db.VBD.get_VDI & Db.VDI.get_sm_config for each VBD & VDI, during each iteration (however we could mostly eliminate the last two by only computing them when necessary and making them lazy). Not sure whether this is a performance issue, and how expensive/slow these DB accesses are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could actually restore the old performance of this function, without using get_info and introducing duplication:

let check_operation_error ~__context ~ref =
  let vmr = Db.VM.get_record_internal ~__context ~self:ref in
  let vmgmr = lazy (maybe_get_guest_metrics ~__context ~ref:(vmr.Db_actions.vM_guest_metrics)) in
  (fun ~op ~strict ->
      let x = lazy ... in
      ...
  )

Then, when we call it once only, the code remains the same. But when we call it in a loop with multiple operations, like in update_allowed_operations, we can do:

let check_operation_error ~__context ~ref = check_operation_error ~context ~ref in
loop (
  ... check_operation_error ~op ~strict:true
)

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

Successfully merging this pull request may close these issues.

3 participants