Opened 7 years ago

Closed 7 years ago

#653 closed defect (fixed)

Harden SimpleTimer2, remove all uses of SimpleTImer

Reported by: zzz Owned by: zzz
Priority: maintenance Milestone: 0.9.3
Component: api/utils Version: 0.9
Keywords: Cc: zab@…
Parent Tickets: Sensitive: no

Subtickets

Attachments (1)

NoMoreST1.txt (79.0 KB) - added by Zlatin Balevsky 7 years ago.
Patch removing all uses of SimpleTimer? and replacing with ST2. Based on forum patch 3

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by zzz

Summary: Harden SImpleTimer2, remove all uses of SimpleTImerHarden SimpleTimer2, remove all uses of SimpleTImer

comment:2 Changed 7 years ago by Zlatin Balevsky

One side effect of migrating to ST2 can be a change in garbage creation rate which can change the frequency of garbage collection pauses. These Hotspot JVM flags can help detect such regression:

-XX:+PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps

Source: http://www.oracle.com/technetwork/java/javase/tech/vmoptions-jsp-140102.html

comment:3 Changed 7 years ago by zzz

Owner: changed from zb to zzz
Status: newassigned

Changed 7 years ago by Zlatin Balevsky

Attachment: NoMoreST1.txt added

Patch removing all uses of SimpleTimer? and replacing with ST2. Based on forum patch 3

comment:4 Changed 7 years ago by zzz

Synch fixes for SimpleTimer2, similar to those in the attachment, included in 0.9.1-15.

Leaving ticket open until the SimpleTimer? users are migrated over.

comment:5 Changed 7 years ago by zzz

Milestone: 0.9.20.9.3

Going through the rest of the patch now, probably to be checked in to my i2p.i2p.zzz.test branch, to be propped back to trunk after 0.9.2 is out.

A large part of the patch is converting SimpleScheduler? to use SimpleTimer2.TimedEvent? instead of SimpleTimer?.TimedEvent?. I don't see a good reason to do that - it breaks the SS API, and SS events can't be cancelled or rescheduled anyway, so there's no benefit. It would, however, be nice someday to get SS and ST2 to use the same ScheduledThreadPoolExecutor?, to save 4 threads. If we could do that without breaking the SS API that would be nice.

So that leaves about 9 files with ST→ST2 replacements.

In ST2 there are existing comments about schedule() vs. reschedule() - it says the former should only be used from the non-scheduling constructor or from timeReached(), and the latter only from outside of timeReached(). Your patch converts new uses to use reschedule() from within timeReached(). Would you please research whether the comments are still true, which to use when and where, is one still more efficient than the other, etc.

comment:6 Changed 7 years ago by Zlatin Balevsky

A large part of the patch is converting SimpleScheduler to use !SimpleTimer2.TimedEvent instead of SimpleTimer.TimedEvent.

Correct, not really necessary; I just wanted to remove all imports of SimpleTimer. Later on I found a nice shortcut: make !SimpleTimer2.TimedEvent implement SimpleTimer.TimedEvent and the imports can go away.


Would you please research whether the comments are still true, which to use when and where, is one still more efficient than the other, etc

This is no longer valid:

/**
         *  More efficient than reschedule().
         *  Only call this after calling the non-scheduling constructor,
         *  or from within timeReached(), or you will get duplicates on the queue.
         *  Otherwise use reschedule().
         */

The only practical difference between the two methods is the useEarliestTime parameter.

comment:7 Changed 7 years ago by Zlatin Balevsky

Cc: zab@… added

ignore, just trying to figure out how to get notifications from trac

comment:8 Changed 7 years ago by Zlatin Balevsky

test, ignore

comment:9 Changed 7 years ago by zzz

Status: assignedaccepted

Thanks.

Comments fixed, SimpleTimer? deprecated, reschedule() changed back to schedule() where inside timeReached() for clarity, all checked in to i2p.i2p.zzz.test, no more SimpleTimer? threads.

Leaving ticket open until prop back to trunk after 0.9.2 release.

comment:10 Changed 7 years ago by zzz

Resolution: fixed
Status: acceptedclosed

Propped in 0.9.2-1.

Note: See TracTickets for help on using tickets.