Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#789 closed enhancement (fixed)

VersionComparator w/o String churn

Reported by: zab Owned by: zab
Priority: minor Milestone: 0.9.4
Component: api/general Version: 0.9.3
Keywords: object churn Cc: zab@…
Parent Tickets:

Description

Profiling data suggests that the StringTokenizer and StringBuilder operations in VersionComparator are a source of object churn.

It can be modified to use manual parsing logic that does not create any temp strings.

all allocations by object size
breakdown of char array allocations

Subtickets

Change History (8)

comment:1 Changed 7 years ago by zzz

Yes it's called a lot lot lot. And creating objects in compare() is a heck of a lot of churn when sorting things.

While almost every use instantiates a new comparator, be careful if you plan to add any fields to the it, as there _may_ be a couple uses that cache a comparator and use it unsynced. Can't remember.

Or, we could go the other way, and add a public static method, and don't instantiate it at all in some of those cases where it's a one-off (not e.g. part of a TreeMap? constructor).

So we should go one way or the other.

Also, we should optimize more for the equals case. e.g. add a lop.equals(rop) check at the top of longCompare.

comment:2 Changed 6 years ago by zab

Before I start rewriting the VersionComparator I'd like to decide what to do with malformatted version strings. Take a look at core/java/test/scalatest/net/i2p/util/VersionComparatorSpec.scala for some examples of strings that I think should throw an IllegalArgumentException but are currently considered valid.

Looks like these strings are stored in the NetDB and therefore need to be sanity-checked before processing, i.e. any entry that does not have a valid version string should be dropped. Are there any arguments for not dropping such versions?

comment:3 Changed 6 years ago by zzz

No we don't want to ever throw an IAE. VersionComparator? is used in many places (not just for router version) and is designed to be "tolerant". Any sanity checking is specific to a particular use case and should be done elsewhere if required.

Throwing an unchecked exception in the comparator could blow up some important thread in the router and be a DoS vector.

Some examples of other current uses: Plugin versions, Jetty version, Java version, Wrapper version. Plugin authors in particular tend to get creative with their version schemes.

comment:4 Changed 6 years ago by zab

Take a look at http://pastethis.i2p/show/2460/

It behaves identically to the current VersionComparator, except for the following cases which are invalid anyway:

"" vs "." : current returns 0, this -1
<non-asccii garbage> vs "" : current returns 0, this -1

All other tests in the VersionComparatorSpec pass.

comment:5 Changed 6 years ago by zzz

looks good to me

comment:6 Changed 6 years ago by zab

  • Resolution set to fixed
  • Status changed from new to closed

Should be in trunk with revision 86448cdee55b7a289c54652fc527ecd4ccc2eb81

comment:7 Changed 6 years ago by zzz

  • Milestone set to 0.9.4

Please restore the top-level VersionComparator? javadocs that got deleted somewhere along the way, thanks.

comment:8 Changed 6 years ago by zab

Javadocs restored in 8649df3d4bd8190c16880ae5eb28a4a9365d4dfc

Note: See TracTickets for help on using tickets.