Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#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: zab@…
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 6 years ago by Zlatin Balevsky

Cc: zab@… added

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.

comment:2 Changed 6 years ago by zzz

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 5 years ago by str4d

Keywords: performance hang cleanup added
Milestone: 0.9.12

comment:4 Changed 5 years ago by tuna

Patch submitted in rev ef4752e434e9bdb82e9a41cb67e2a8ccead202a8 on i2p.i2p.tuna.tmp

comment:5 Changed 5 years ago by zzz

Milestone: 0.9.20
Owner: set to tuna
Status: newassigned

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 5 years ago by zzz

Also, refer users to the replacement ST2 in the SS javadocs/deprecations

comment:7 Changed 5 years ago by tuna

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 5 years ago by tuna

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

Version 0, edited 5 years ago by tuna (next)

comment:11 Changed 5 years ago by tuna

Resolution: fixed
Status: assignedclosed

Propagated from i2p.i2p.tuna.tmp to i2p.i2p

comment:12 Changed 5 years ago by zzz

In 0.9.19-2.

Only plugin I see using SS is zzzot, to be fixed by me. See #1523

Note: See TracTickets for help on using tickets.