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

Refactor Api::Environment #14986

Merged
merged 7 commits into from
May 11, 2017

Conversation

imtayadeway
Copy link
Contributor

Took some suggestions out of #14624 and bundled them with some other refactorings. The result is a little longer, but I think it adds clarity and consistency.

Among the changes:

  1. Got rid of an unnecessary begin block
  2. Implement the normalized_attrs data using sets instead of hashes (we don't use the values, we just assign all values to true
  3. Remove the normalized_attrs hash altogether, which should lower the memory footprint of this module, since it duplicated some other data which never got GC'd
  4. Provide a more consistent API for checking if an attribute needs to be normalized with some top-level Api.<type>_attribute? helper methods

@Fryguy thanks for some of these suggestions!
@miq-bot add-label api, refactoring
@miq-bot assign @abellotti

@abellotti
Copy link
Member

@imtayadeway can you run a performance test with before (hash) and after (set), of GET 1000 VM's with expand=resources. Thanks.

@miq-bot
Copy link
Member

miq-bot commented May 5, 2017

This pull request is not mergeable. Please rebase and repush.

@Fryguy
Copy link
Member

Fryguy commented May 5, 2017

😍 I love it!

@miq-bot
Copy link
Member

miq-bot commented May 5, 2017

Checked commits imtayadeway/manageiq@3742b42~...f1a3b03 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@imtayadeway
Copy link
Contributor Author

@abellotti OK I have some numbers for you! Couldn't quite get to 1000 but I had 714 Vms in my database so I limited to 700 in my query. I ran the query 20x on each run (after warming up) so hopefully any big quirks will have been somewhat evened out:

Here's before:

tim@tim-thinkpad:~/src/manageiq$ time for i in `seq 1 20`; do curl --user admin:smartvm 'localhost:3000/api/vms?expand=resources&limit=700' &> /dev/null; done

real	8m51.058s
user	0m0.084s
sys	0m0.028s

And after:

tim@tim-thinkpad:~/src/manageiq$ time for i in `seq 1 20`; do curl --user admin:smartvm 'localhost:3000/api/vms?expand=resources&limit=700' &> /dev/null; done

real	8m47.522s
user	0m0.068s
sys	0m0.052s

So the data says: slightly faster ;) Pinch of salt though, I wouldn't want to seriously make that claim - there are many extraneous things that could account for that. Importantly though, I think we can say it's in the same ball park. LMK if I can do anything else!

@abellotti
Copy link
Member

This is fine @imtayadeway same ballpark, less in user more in system. Thanks for running the tests 🎵

@abellotti
Copy link
Member

LGTM !! 👍

@abellotti abellotti merged commit 2420c03 into ManageIQ:master May 11, 2017
@abellotti abellotti added this to the Sprint 61 Ending May 22, 2017 milestone May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants