RFR: 8239235: Examine SignatureStream performance after consolidation

[Claes Redestad]

Hi Lois,

On 2020-02-20 23:22, Lois Foltan wrote:
> 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 ';'?

right, doesn't matter, just thought the code reads better without a
standout embedded increment.

> - 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");

OK - also refactored to not fall through from T_ARRAY to the primitive
case, and added two different asserts for the subtly different cases.

> - 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.

Queued up more extensive testing.

> 
> 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!

Done - and thanks!

/Claes