RFR: 8239235: Examine SignatureStream performance after consolidation

[Lois Foltan]

On 2/20/2020 12:53 PM, Claes Redestad wrote:
> Hi,
>
> JDK-8230199 reduced some of the work related to signature parsing, but
> also caused a small increase in branches and branch-misses on select
> startup tests.
>
> This patch addresses this by means of more aggressive inlining and
> a few simplifications and optimizations, adding up to a small reduction
> of retired instructions, branches and branch misses on various tests.
> Every metric now seems as good as or better than the pre-JDK-8230199
> baseline.
>
> Even with more aggressive inlining the net result on static binary size
> is perhaps surprisingly a slight positive (-4kb - linux x64).
>
> I also found a subtle bug in SignatureStream::find_symbol where we would
> drop/leak the initial, non-permanent Symbol in a signature if it was
> followed by a permanent one. I discussed whether we should break this
> out into a separate bug with Lois, but decided to not break it out. I've
> added a regression gtest for this particular issue, though.
>
> Bug:    https://bugs.openjdk.java.net/browse/JDK-8239235
> Webrev: http://cr.openjdk.java.net/~redestad/8239235/open.00/
>
> Testing: tier1-2 (did tier1-4 on a previous version along with other
> patches)
>
> /Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8230199

Hi Claes,

Overall this looks good.  Thank you for doing these changes! I have some 
comments:

runtime/signature.cpp:
- line #226: the if statement was changed to not post increment end?  I 
assume this is because  it does not matter if memchr scans the 'L' again 
when it looks for the ';'?
- line #230: can you add an assert immediately before line 230 just to 
make sure that a reference type is never encountered past the switch 
statement, something like assert(!is_reference_type(type), "only 
primitive types expected");
- I think the changes you made to SignatureStream::next() are fine but 
can you make sure you run hs-tier1 through 8?  I had JCK test failures 
within SignatureStream::next() if I recall in hs-tier5 and 6.  So all 
tiers should be run for this change.

Clever to initialize SignatureStream::_previous_name with j.l.Object!  
Please don't forget to update copyrights and thanks for adding a test 
for that bug!

Thanks,
Lois