RFR: JDK-8145564: 8036003: startup regression on linux fastdebug builds

[Magnus Ihse Bursie]

On 2015-12-17 22:32, David Holmes wrote:
> On 17/12/2015 7:24 PM, Erik Joelsson wrote:
>> On 2015-12-17 09:08, David Holmes wrote:
>>> On 17/12/2015 5:54 PM, Erik Joelsson wrote:
>>>>
>>>>
>>>> On 2015-12-17 01:40, David Holmes wrote:
>>>>> On 17/12/2015 7:35 AM, Erik Joelsson wrote:
>>>>>> One more thing, where should this fix be pushed? Do you need it
>>>>>> urgently
>>>>>> in hs-rt?
>>>>>
>>>>> It is urgently needed in both the hs-rt and hs-comp repos as it
>>>>> affects nightly testing and JPRT. If Alejandro agrees I suggest
>>>>> pushing to jdk9/hs and it can then be pulled down to the team repos.
>>>>>
>>>> Will do.
>>>>> With regard to the fix ... previously DEBUG_BINARIES was never set in
>>>>> spec.gmk and so was never externally introduced into the hotspot 
>>>>> build
>>>>> this way. So why not simply change the name of the variable so 
>>>>> that it
>>>>> has no affect on the hotspot part of the build?
>>>>>
>>>> Well, we don't want it affecting the jdk part of the build either at
>>>> this point. This patch aims to restore the "external" and "zipped"
>>>> settings to exactly what they were before the patch, as was promised.
>>>
>>> I had not inferred from any of this that what was being done via
>>> NativeCompilation.gmk was in any way a problem. I would have expected
>>> any problems there to be readily seen before this was reviewed and
>>> pushed. So I'm a bit confused about this.
>>>
>> I didn't follow this particular review closely as Magnus was on it and
>> so I had missed the NativeCompilation part. It's true that it is very
>> unlikely to be part of the problem described in this bug, but I still
>> feel that the safest action at this point is to restore the behavior of
>> "external" and "zipped" to what they used to be.
>
> My concern is recognizing that these adjustments do in fact restore 
> the old behaviour. From the hotspot side it seemed cleaner to avoid 
> the breakage by using a different variable name.

I'm not sure about this talk about a different variable name? The 
*point* of the patch was *exactly* to set DEBUG_BINARIES=true for 
internal builds. The problem was that it also set it for 
external/zipped, which was incorrect. But neither the writer of the 
patch nor me (or any other reviewer) realized that this would have this 
"hammer" effect.


>
>> Magnus is working on a
>> complete fix where debug symbols will be enabled for everything in
>> NativeCompilation.
>>> I'm tempted to say rollback the whole thing rather than hack at it.
>>>
>> Rolling back will be much more work for me than submitting this patch.
>> There are two changes already that depend on this change. I also don't
>> dislike the idea of the new configure parameter.
>
> Rolling back the new configure parameters and then reinstating them 
> again would also not win us any friends as it would be very 
> disruptive. As others have noted the way we introduce and remove 
> configure parameters needs to be looked at.
This makes it sound like we're sloppy with configure arguments when we 
actually try hard not to be.

Normally, we don't have any problems with introducing new configure 
arguments. Most often, when we introduce a new argument, the behaviour 
falls back to the old behaviour as default if the argument is not 
specified. From time to time, we actually need to change the behaviour 
of configure, and this does not really apply. But then again, all 
modification somehow change behaviour regardless of if any options are 
changed, an all changes in behaviour breaks someones workflow 
(compulsory xkcd reference: https://xkcd.com/1172/).

In this case, not setting the new configure option resulted in the old 
default behaviour.

As for removing options, we are more conservative. We never remove 
options, we just deprecate them. (With a few exceptions, e.g. sometimes 
we have introduced temporary options which are clearly announced as 
such.) Our plan is to remove deprecated options once some time has 
passed (e.g. next major release) but, like the Java language itself, so 
far we have not actually removed any deprecated options. :-)

When we deprecate an option, we ignore the value it sets, but prints a 
warning stating that the option is deprecated. The configure call will 
not fail, though. The warning is repeated as the very last thing so it 
should be easy for the user to spot, like this:

The following warnings were produced. Repeated here for convenience:
WARNING: Option --with-override-jaxp is deprecated and will be ignored.

In some cases we've tried to write some "glue" to interpret old and 
deprecated options in terms of new. (I only think we've done this in the 
closed source). I don't really like it. It means a lot of tests, 
handling situations like what if both old and new are set, and they 
conflict? Or if both old and new are set and they do *not* conflict? 
What if the behaviour has changed slightly, so it does not really match? 
When should we stop helping the user to convert from the old to the new?

> I was mainly concerned that the out-of-the-box default behaviour was 
> unchanged.

The stated goal of the patch was that out of the box behaviour should 
have been unchanged. The patch was however incorrect, and this fact was 
not detected during testing of the patch (I only tested if it built ok, 
not that the resulting build did not degrade in performance) nor during 
code review. Such things happen, and then you need to fix them.

/Magnus

>
>>> And apologies as I'm just about to go offline for a few hours at least.
>>>
>> I hope you won't object to me pushing this with just ihse as reviewer. I
>> feel this is rather a priority to get fixed.
>
> Sure. I had verified that DEBUG_BINARIES is only ever tested against 
> "true" so setting it to false should be as effective as not setting it 
> at all.
>
> I'll follow up separately to see where this was pushed and whether we 
> need to pull it into anywhere else urgently.
>
> Thanks,
> David
>
>> /Erik