Name.contentEquals() performance

[Maurizio Cimadamore]

Hi,
If you are comparing names (to make sure of which requires an 
instanceof, which I guess adds its own cost), the interning trick seems 
to be the best IMHO. I'm a bit skeptical about adding a new field on 
Name; a simple compilation of an hello world program will create roughly 
3K names. In general you need a new name for each new field, variable, 
method, class/interface, type-variable, ... so the number of names can 
become big pretty rapidly.

(That said, out of curiosity I tried to instrument the JDK build to see 
how many names were created (this is a bit hard given that the build 
internally reuses java compilers using the sjavac server), but name 
creation seem to peak at around 33K, which would give an overhead of a 
couple of hundred of kB for an extra field like the one you suggest, 
which doesn't seem excessive in terms of footprint increase)

So, given the performance improvement you get out of this is rather 
limited, perhaps going for a more conservative fix (e.g. interning) 
could be the better?

As for your claim of less retained memory it's really hard to tell w/o 
looking at some targeted JMH benchmark - it's possible that the JIT 
might decide to escape analyze away the strings since they are thrown 
away immediately after the comparison, so that no real allocation 
actually occurs? In any case I would expect modern GCs to be able to 
cope quite well with this kind of throwaway allocation with TLABs etc. 
so I'm a bit on the fence.

Maurizio



On 03/10/18 12:20, Ron Shapiro wrote:
> In my profiling of javac compilations of large builds (>30 seconds), 
> I'm seeing about a noticeable amount of time being attributed to 
> com.sun.tools.javac.util.Name.contentEquals(), because it creates a 
> new String each time in it's toString().equals(cs.toString()) check. 
> I'm wondering if we can optimize this without much cost.
>
> I had suggested to Liam that we could add an `asString` variable that 
> is lazily computed in toString(). This would also benefit all of the 
> other CharSequence methods that delegate to the toString() representation.
>
> Liam had another idea that would avoid adding a field (and the 
> associated memory that comes with that), taking advantage of the fact 
> that Names are interned.
>
> if (cs instanceof Name) {
>   Name other = (Name) cs;
>   if (table == other.table) {
>     return this == other;
>   }
> }
> return toString().equals(cs.toString());
>
> The first option reduced builds that I tested of 45 seconds by 450ms 
> (1%). Liam's suggestion reduced bulilds by 400-420ms, so in the same 
> ballpark but a bit slower (but again, it allows for less retained 
> memory?).
>
> I figured both are simple, so I'd propose both.