#1069 closed enhancement (fixed)
Combine SimpleScheduler and SimpleTimer2 threads
Reported by: | zzz | Owned by: | tuna |
---|---|---|---|
Priority: | minor | Milestone: | 0.9.20 |
Component: | api/utils | Version: | 0.9.8.1 |
Keywords: | performance hang cleanup | Cc: | Zlatin Balevsky |
Parent Tickets: | Sensitive: | no |
Description
Can we combine the backend ThreadPoolExecutors? for SimpleScheduler? and SimpleTimer2 to go from 8 threads to 4? We would keep the same classes and front-end interface, just execute them in the same pool.
Subtickets
Change History (12)
comment:1 Changed 7 years ago by
Cc: | Zlatin Balevsky added |
---|
comment:2 Changed 7 years ago by
The proposal in the OP is messy.
Perhaps a better one is:
- Have ST2 support the SS methods, i.e.:
- Add the addEvent() and addPeriodicEvent() methods to ST2. Maintain the SimpleTimer?.TimedEvent? parameter
- In addEvent(), wrap the ST.TE class in a TimedEvent?
- In addPeriodicEvent(), wrap the ST.TE class in a PeriodicTimedEvent? extends TimedEvent?
- Convert all the in-tree calls from _context.simpleScheduler() to _context.simpleTimer2()
- Deprecate I2PAppContext.simpleScheduler()
comment:3 Changed 6 years ago by
Keywords: | performance hang cleanup added |
---|---|
Milestone: | 0.9.12 |
comment:4 Changed 6 years ago by
Patch submitted in rev ef4752e434e9bdb82e9a41cb67e2a8ccead202a8 on i2p.i2p.tuna.tmp
comment:5 Changed 6 years ago by
Milestone: | → 0.9.20 |
---|---|
Owner: | set to tuna |
Status: | new → assigned |
review:
- in the new SimpleTimer2 methods, you must add the events to the correct SimpleTimer2, i.e. "this", not SimpleTimer2.getInstance()
- would be less code to just call the 3-arg addPeriodicEvent() from the 2-arg addPeriodcEvent(), as was done in SimpleScheduler?. You could then get rid of one of the PTE constructors.
- Add @since to new methods
- No need to deprecate private methods, although I suppose it doesn't hurt if it doesn't generate more warnings
- Not sure why you did an inline class on top of an abstract class instead of just making the PTE class concrete, to me it's less readable, but if you're used to doing Android coding it's awfully familiar. Up to you.
- If you haven't, you'll probably need to test with logging on to make sure the scheduled tasks are executed when expected. It may not be apparent if things don't work, since scheduled stuff is generally for data structure cleanups and other minor things.
- Probably need to analyze whether it's necessary to raise the number of ST2 threads to handle the additional work.
- I never did understand zab's comment 1 above. Hopefully you do, and have researched it to your satisfaction.
comment:6 Changed 6 years ago by
Also, refer users to the replacement ST2 in the SS javadocs/deprecations
comment:7 Changed 6 years ago by
1) Fixed
2) Fixed
3) Fixed
4) No new warnings generated, keeping the @deprecated
5) I can't say I have much of an Android background, but if keeping it an inline class is ok, I'll do that.
6) Remains to be done
7) Remains to be done
9) No, I don't understand zabs comment. Maybe he was referring to rescheduling from run method.
10) Fixed
comment:10 Changed 6 years ago by
I measured the difference in scheduled time and time at which event is executed under the patched and unpatched versions.
The test was run for 69minutes, catching all the events (~2700 of them) that were once handled by SS, on a 10k tunnel router.
patched
95th: 2.000000ms
Avg: 2.434004ms
unpatched
95th: 1.000000ms
Avg: 2.662817ms
comment:11 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Propagated from i2p.i2p.tuna.tmp to i2p.i2p
comment:12 Changed 6 years ago by
In 0.9.19-2.
Only plugin I see using SS is zzzot, to be fixed by me. See #1523
Do we know each and every Runnable that will ever execute on SimpleScheduler and SimpleTimer2? If not, the only way to find the answer is to try and see what happens. In the worst case scenario, this will cause a deadlock or a halt.