On Mon, Apr 23, 2018 at 7:42 PM, Isaac Levy <isaac.r.l...@gmail.com> wrote: > On Mon, Apr 23, 2018 at 5:18 PM David Lloyd <david.ll...@redhat.com> wrote: >> >> FWIW I strongly doubt this will improve performance; probably the >> opposite in fact, as IIRC an enum switch generates an extra class >> (though perhaps this has changed). The original code was quite >> compact and utilized identity comparisons, and given there are only >> three alternatives it probably was able to exploit branch prediction >> as well (if such a thing even matters in this context). > > > Well, there are enum switches on the same enum elsewhere in the class, so > should those instead be replaced by if checks?
I think that any change that's made for performance should be tested against performance regression, generally speaking. My personal understanding is that, when there are a small number of alternatives, an identity "if" tree can perform slightly better in some cases because HotSpot C2 gathers and utilizes statistics about branches taken in these cases; for switch, it (still IIRC) does not do so. Given that this is regex, it might be worth testing. > A larger change could remove this branch entirely, with different classes for > each of the types, which > are known during compile. If that is beneficial. However every new class adds metaspace usage and (in this case) some kind of polymorphic dispatch, so you'd want to be fairly confident that in a typical application, only one of the two would be loaded, or that there would be some other significant gain, as there is definitely a cost. Everything has a cost, especially in hot perf-sensitive code. Anyway I'm not a reviewer but these are considerations that I would have were I contributing the change. JMH might be of use. -- - DML
