Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#787 closed enhancement (fixed)

Object churn optimization in RateStat

Reported by: Zlatin Balevsky Owned by:
Priority: trivial Milestone:
Component: api/general Version: 0.9.3
Keywords: object churn Cc: zab@…, dev@…
Parent Tickets: Sensitive: no

Description

Trivial change to not create a ton of Iterator and Long objects when calling RateStat.addData(..) . http://pastethis.i2p/show/2287/

Subtickets

Change History (17)

comment:1 Changed 7 years ago by zzz

Component: router/generalapi/general

You think all that is worth it? Remember default is no full stats, there's only a couple dozen stats in the default setup.

comment:2 Changed 7 years ago by zzz

On further reflection, I think it is worth it. But wouldn't a final ArrayList? be better than a volatile array for the rates?

Also, there should be zero actual cases where createRateStat() is called a second time with different Rates (Longs) than the original call, so that code path, while necessary I guess, isn't so important to optimize.

There are of course stats where createRateStat is called repeatedly with the same args. I've been cleaning that up over the years. I'll put in some logging and see if I can fix up more of them, as that call is a little expensive.

comment:3 Changed 7 years ago by zzz

Hmm. Actually, createRateStat() doesn't even attempt to add the missing rates. Therefore RateStat?.addRate() is unused. So really not worth a lot of effort to make it efficient.

comment:4 Changed 7 years ago by Zlatin Balevsky

Re: ArrayList vs Rate[] : iterating over an ArrayList will create an Iterator object each time which defeats the purpose. Of course, it's more convenient and we can add the CachedIteratorArrayList hack I had in an unrelated patch (to PeerState IIRC) a few months ago.

Re: createRateStat & addRate I implemented those for completeness; addRate appears to be used when de-serializing rates from disk? Not sure if that ever happens so whatever.

Let me know how to proceed. I think I'll add the CachedIteratorArrayList regardless because it's useful in many places.

comment:5 Changed 7 years ago by zzz

Cc: dev@… added

Grepping the whole tree for addRate:

./core/java/test/junit/net/i2p/stat/RateStatTest.java:        rs.addRate(1000);
./core/java/src/net/i2p/stat/RateStat.java:    public void addRate(long period) {

However after researching mtn history, hottuna (dev@…) changed from a Rate[] array to a ConcurrentHashMap? and added addRate() and removeRate() in July of last year:

Revision: b768f0483dce793b4654191c2e649de04172087e
Parent:   13af6dbeb49a0ab1855da31595ba3c141e54bc59
Author:   dev@robertfoss.se
Date:     07/20/2011 01:27:44 PM
Branch:   i2p.i2p

Changelog: 

Added support for adding/removing periods to already existing RateStats.

Changes against parent 13af6dbeb49a0ab1855da31595ba3c141e54bc59

  patched  core/java/src/net/i2p/stat/Rate.java
  patched  core/java/src/net/i2p/stat/RateStat.java

I think it might have been related to his i2pcontrol / itoopie work that summer, but I can't find any use of addRate() or removeRate() in the i2pcontrol or itoopie source.

Deserializing uses load().

Not sure about the idea of using the new CachedIteratorArrayList?. I don't like storing the Rates twice (both in a ArrayList? and ConcurrentHashMap?, or ArrayList? and TreeMap?, or Rate[] and TreeMap?).

A CIAL would require us to synchronize everything. Since addRate() is unused, we don't need synchronization otherwise.

At this point I think we need guidance from hottuna on the purpose of his changes. Also, reviewing the mtn history is instructive. If hottuna's changes weren't really necessary, perhaps we can just revert them (more or less) to get back to the Rate[] array.

adding tuna to CC.

comment:6 Changed 7 years ago by zzz

One thing I didn't make clear above. There are a LOT of RateStats?, even if full stats are disabled, because each peer profile has several. We must be careful not to add unnecessary objects into RateStat?. Another reason I'm not thrilled about the array → ConcurrentHashMap? change of tuna's.

comment:7 Changed 7 years ago by Zlatin Balevsky

Re: synchronization - right now (as in paste 2287) I'm doing copy-on-write so that should work fine even if addRate & removeRate are used, as long as it's not used too often.

Re: sizeof(RateStat) - two arrays (one Rate[], one long[]) is less than a ConcurrentHashMap

comment:8 Changed 7 years ago by DISABLED

addRate/removeRate are not frequently called.
ConcurrentHashMap? was chosen due to it being guaranteed to work. Concurrency issues would be acceptable and not have any catastrophic consequences for I2Pcontrol.

Regarding removing the functions entirely, that would be pretty bad for the stats-part of I2Pcontrol.
That being said, is anyone actually using i2pcontrol? Maybe ciuci(?)?

tuna

comment:9 Changed 7 years ago by zzz

@tuna To repeat from my comment 5 above, where is addRate/removeRate being used and why? What was the purpose of these changes? I can't find any usage in i2p.itoopie or i2p.plugins.i2pcontrol.

My head for i2p.plugins.i2pcontrol is: ad8923c7c2573de22db3c89c34baff0f4362b0fa dev@… 08/28/2012

comment:10 Changed 7 years ago by DISABLED

Hmm, it seems you are right zzz. Then we can just remove it?
Sorry about the confusion.

tuna

comment:11 Changed 7 years ago by zzz

@zab with that revelation from tuna, what's the best way to proceed? Seems like getting rid of the CHM (i.e. more or less reverting tuna's change from July 2011) would be a good start?

comment:12 Changed 7 years ago by Zlatin Balevsky

@zzz:
in the RateStat.load(..) method there is a catch(IllegalArgumentException) clause which adds a new Rate object if the Rate.load(..) method fails to deserialize. Because of that we still need the ability to at least add new Rates, so http://pastethis.i2p/raw/2287/ minus the removeRate method.

If we know for sure this exception will never happen, then a simplified version http://pastethis.i2p/show/2447/ will do the job.

comment:13 Changed 7 years ago by zzz

The only user of RateStat?.load() I know of is ProfilePersistenceHelper?.

It's very odd that RateStat?.load() is declared to throw an IAE but catches it and fixes up an individual rate within the file if it is corrupt, and thus never really throws an IAE. It's been that way since the first checkin (April 2004). And looking at Rate.load(), it only throws an IAE if the period is ⇐ 0.

What Rate.load() should really do is verify that the read-in period matches the period from the constructor as you really don't want to change the locally stored period to be different from what RateStat? thinks it is (especially if RateStat? is storing it in a separate array). _period should be final.

Since ProfilePersistenceHelper? is clearly prepared to handle an IAE since it is checked, seems to make sense to just let it throw out of RateStat?.load() instead of catching it there.

So I like paste 2447.

Some suggested mods to 2447:

  • Leave addRate() and removeRate() in there, but deprecate, and throw an UnsupportedOperationException?. Or maybe throw an UOE for add and just make remove a no-op.
  • We could just throw an IAE in the constructor if periods.length == 0 and remove the checks elsewhere
  • Looking back to what was there before tuna's change b768f0483dce793b4654191c2e649de04172087e, getRate() was a linear search, without a local array for the periods:
    -        for (int i = 0; i < _rates.length; i++) {
    -            if (_rates[i].getPeriod() == period) return _rates[i];
    

If you think a local array and a binary search is better, ok. Just wanted to point out what was there before.

  • Stylistically, importing static Arrays.* to get sort and binarySearch and copyOf is a little confusing but maybe that's just me.

I don't know if there's a good test case for all this. If not, all the peer profiles are read in at router startup (only).

comment:14 Changed 7 years ago by Zlatin Balevsky

Check out http://pastethis.i2p/show/2463/

Includes some of the comments you provided on 2447:

  • {add,remove}Rate are there but deprecated and throwing an UOE
  • constructor throws IAE if periods.length == 0
  • removes the long[] _periods field, reducing memory footprint!
    • because of the above, we're back to linear scan in getRate() & containsRate()
    • however, getPeriods() does have some object churn. It is used in StatisticsManager.publishStatistics()
  • stylistically re: static imports - don't care either way works for me

comment:15 Changed 7 years ago by zzz

looks good to me

comment:16 Changed 7 years ago by Zlatin Balevsky

Resolution: fixed
Status: newclosed

should be in trunk with revision 9ee7a0a59e970eaee77643d1affb278d0793a727

comment:17 Changed 7 years ago by zzz

Some duplicate createRateStat calls (see comment 2 above) fixed in 0.9.4-2.

Note: See TracTickets for help on using tickets.