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

Fixes #1592: making report::expire faster and without errors #1659

Closed
wants to merge 1 commit into from

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Aug 7, 2014

No description provided.

@orrabin
Copy link
Member Author

orrabin commented Aug 7, 2014

I'd like to test it on real reports but for now on 5000 generated reports I used:
puts Benchmark.measure {Report.expire()}
and got these results:

before change:
2014-08-07 10:20:09 +0300: Expired 5000 Reports
0.470000 0.000000 0.470000 ( 1.963282)

after change:
2014-08-07 09:52:39 +0300: Expired 5000 Reports
0.020000 0.010000 0.030000 ( 0.483464)

@domcleal
Copy link
Contributor

domcleal commented Aug 7, 2014

Something else to measure will be the memory usage, as that's been a common complaint about reports:expire and it looks like the original code was attempting to keep it down (successfully or not, I'm unsure). /bin/time can measure max resident size, which may be useful.

@orrabin
Copy link
Member Author

orrabin commented Aug 7, 2014

@domcleal I used /bin/time as you suggested and the max resident size seems to be better after my changes. Are there more tests I can run?

The results:
before change:
14.95user 0.90system 0:17.39elapsed 91%CPU (0avgtext+0avgdata 751664maxresident)k
0inputs+1328outputs (0major+118227minor)pagefaults 0swaps

after change:
14.14user 0.91system 0:15.53elapsed 96%CPU (0avgtext+0avgdata 584480maxresident)k
0inputs+64outputs (0major+105726minor)pagefaults 0swaps

all_sources = Source.pluck(:id)
orphaned_sources = all_sources - used_sources
Source.where(:id => orphaned_sources).delete_all unless orphaned_sources.empty?
count = 0
Copy link
Member

Choose a reason for hiding this comment

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

Why initializing it to 0?

count = Report.where(cond).delete_all should do.

@dLobatog
Copy link
Member

dLobatog commented Aug 7, 2014

👍 except for the code nitpicks, even if it was a tad slower I'd favor this as it's considerably simpler to understand and maintain.

@orrabin
Copy link
Member Author

orrabin commented Aug 7, 2014

@elobato sorry, those where leftovers from the previous code.
Fixed the nitpicks :)

@dLobatog
Copy link
Member

dLobatog commented Aug 7, 2014

I'm testing memory usage for 10k reports before merging.

@orrabin
Copy link
Member Author

orrabin commented Aug 7, 2014

@elobato thanks!

@dLobatog
Copy link
Member

dLobatog commented Aug 7, 2014

Peak mem usage seems to be even lower here than in the old version -

Merged as 4215def , thanks @orrabin !

all_sources = Source.pluck(:id)
orphaned_sources = all_sources - used_sources
Source.where(:id => orphaned_sources).delete_all unless orphaned_sources.empty?
Message.joins(:logs).joins(:reports).where(cond)
Copy link
Member

Choose a reason for hiding this comment

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

unused query

@orrabin
Copy link
Member Author

orrabin commented Aug 9, 2014

@elobato these are the test results for 10k

before change:
2014-08-09 19:54:08 +0300: Expired 10000 Reports
1.050000 0.020000 1.070000 ( 4.305314)
17.27user 1.19system 0:20.93elapsed 88%CPU (0avgtext+0avgdata 661616maxresident)k
0inputs+1456outputs (0major+109556minor)pagefaults 0swaps

after change:
2014-08-09 20:39:47 +0300: Expired 10000 Reports
0.080000 0.000000 0.080000 ( 12.673412)
13.98user 0.82system 0:22.16elapsed 66%CPU (0avgtext+0avgdata 591392maxresident)k
80inputs+72outputs (1major+106172minor)pagefaults 0swaps

@ohadlevy
Copy link
Member

ohadlevy commented Sep 4, 2014

[test]

@orrabin
Copy link
Member Author

orrabin commented Sep 10, 2014

[test]

@orrabin
Copy link
Member Author

orrabin commented Sep 14, 2014

@elobato can you please recheck this?

@ohadlevy
Copy link
Member

merged as ab866a3 thanks @orrabin

@ohadlevy ohadlevy closed this Sep 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants