RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS

Jeremy Manson jeremymanson at google.com
Fri Jul 10 00:23:52 UTC 2015 

Hey Dmitry,

Thanks for looking at it.  The leak was deliberate (under the theory that
the command line flags were leaked, too), but it was silly that I didn't
fix it.

Anyway, fixed.  See:

http://cr.openjdk.java.net/~jmanson/8079301/webrev.03

Jeremy

On Thu, Jul 9, 2015 at 2:52 PM, Dmitry Dmitriev <dmitry.dmitriev at oracle.com>
wrote:

> Hello Jeremy,
>
> Correct me if I am wrong, but I see small memory leak in your
> implementation.
> In Arguments::parse_options_environment_variable function memory for
> 'buffer' allocated by strdup function on line 3415, but now it not freed in
> this function because data from this buffer is used later. On the other
> hand when we are done with env variables, we free only options array(lines
> 3703-3705) and not free previously allocated memory for 'buffer' because we
> lost track for it.
>
> Thanks,
> Dmitry
>
>
> On 09.07.2015 21:20, Jeremy Manson wrote:
>
>> Thanks for looking at this, Coleen.
>>
>> I merged this with the latest version of hs-rt, but Ron is or I am going
>> to
>> have to do some merging for 8061999.  The changes are relatively
>> orthogonal, but they touch a couple of the same places.  How far away is
>> Ron's checkin?
>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~jmanson/8079301/webrev.01/
>>
>> Updated test:
>>
>> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.01/
>>
>> (Mild complaining on my part: the conflicts wouldn't have been an issue if
>> someone had looked at this two months ago, when I first asked.  I suspect
>> Ron hadn't even started his change at that point.  And if external
>> developers were able to submit changes to Hotspot, we wouldn't have had to
>> bother Oracle engineers at all.  Neither of these is your fault or your
>> problem, of course - I'm grateful you were able to take a look.)
>>
>> Jeremy
>>
>> On Wed, Jul 8, 2015 at 6:19 AM, Coleen Phillimore <
>> coleen.phillimore at oracle.com> wrote:
>>
>>  I'll sponsor it but can you repost the RFR?
>>>
>>> There has been a fair bit of work to command line processing lately so
>>> I'd
>>> like to have the others look at it too.  Have you merged this up with the
>>> latest version of hs-rt repository and were there conflicts?   Also, have
>>> you looked at the RFR:
>>>
>>> "Open code review for 8061999 Enhance VM option parsing to allow options
>>> to be specified"
>>>
>>> and are there conflicts with this?
>>>
>>> The hotspot change looks good to me.
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html
>>>
>>> "UseCerealGC" LOL
>>> Can you use a different option than PrintOopAddress in this test?  I
>>> don't
>>> know why this command line option exists or would be useful for, and
>>> seems
>>> to be a good candidate for removal.   If you need a valid argument, that
>>> is
>>> unlikely to go away, TraceExceptions would be good.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 7/7/15 8:00 PM, Jeremy Manson wrote:
>>>
>>>  No love?  Is there a code owner here I should pester more directly?
>>>>
>>>> Jeremy
>>>>
>>>> On Fri, Jun 26, 2015 at 3:00 PM, Martin Buchholz <martinrb at google.com>
>>>> wrote:
>>>>
>>>>   As usual with Google patches, they are in use locally.  This one has
>>>> been
>>>>
>>>>> stable for a while without complaints.  Please sponsor.
>>>>>
>>>>> On Fri, Jun 26, 2015 at 1:53 PM, Jeremy Manson <
>>>>> jeremymanson at google.com>
>>>>> wrote:
>>>>>
>>>>>   All this talk about IgnoreUnrecognizedVMOptions reminded me that this
>>>>>
>>>>>> review is still outstanding.  Any takers?
>>>>>>
>>>>>> Jeremy
>>>>>>
>>>>>> On Tue, May 5, 2015 at 10:01 AM, Jeremy Manson <
>>>>>> jeremymanson at google.com
>>>>>> wrote:
>>>>>>
>>>>>>   Hi David,
>>>>>>
>>>>>>> Thanks for taking a look.
>>>>>>>
>>>>>>> On Mon, May 4, 2015 at 8:32 PM, David Holmes <
>>>>>>> david.holmes at oracle.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>   Hi Jeremy,
>>>>>>>
>>>>>>>> On 5/05/2015 6:51 AM, Jeremy Manson wrote:
>>>>>>>>
>>>>>>>>   Not sure who might be willing to sponsor this.  David?  You did
>>>>>>>> the
>>>>>>>>
>>>>>>>>> last
>>>>>>>>> one.  :)
>>>>>>>>>
>>>>>>>>>   Might be a while before I can get to it. I did have a quick look
>>>>>>>>>
>>>>>>>> (and
>>>>>>>> will need a longer one).
>>>>>>>>
>>>>>>>>  I understand.  I'm just happy it's possible to upstream this stuff
>>>>>>> at
>>>>>>> all[1].
>>>>>>>
>>>>>>> [1] With the eternal caveat that I'd be happier if we didn't *have*
>>>>>>> to
>>>>>>> go through you folks, but we've learned to live with it.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>     Context: A number of command line options are not settable via
>>>>>>>
>>>>>>>> JAVA_TOOL_OPTIONS and _JAVA_OPTIONS:
>>>>>>>>>
>>>>>>>>>      - PrintVMOptions
>>>>>>>>>
>>>>>>>>>   So you have changed the semantics of this to print out the
>>>>>>>>> options
>>>>>>>>>
>>>>>>>> from
>>>>>>>> the command-line and each of the two env vars. Seems reasonable but
>>>>>>>> may be
>>>>>>>> better to know which option came from where as we can now see the
>>>>>>>> same
>>>>>>>> option (or conflicting variants thereof) multiple times.
>>>>>>>>
>>>>>>>>   I'd be happy to do this.  Any preferred syntax?  Does someone
>>>>>>>>
>>>>>>> actually
>>>>>>> own this feature?
>>>>>>>
>>>>>>> I'm unclear what the current processing order is for the different
>>>>>>>
>>>>>>>  sources of options, and whether the changes affect that order?
>>>>>>>>
>>>>>>>>   Nope.  I go through sad contortions to keep the processing order
>>>>>>>> the
>>>>>>>>
>>>>>>> same.  It's JAVA_TOOL_OPTIONS, then command line, then _JAVA_OPTIONS.
>>>>>>> See
>>>>>>> lines 2549-2567.
>>>>>>>
>>>>>>>
>>>>>>>        - IgnoreUnrecognizedVMOptions
>>>>>>>
>>>>>>>>      - PrintFlagsInitial
>>>>>>>>>
>>>>>>>>>   Unclear what "initial" actually means - is it the default?
>>>>>>>>>
>>>>>>>> More or less.  If you stick, for example,
>>>>>>> IgnoreUnrecognizedVMOptions
>>>>>>> in
>>>>>>> there first, it will get printed out as "true".  I haven't really
>>>>>>> changed
>>>>>>> the semantics here either - I've just added processing of the
>>>>>>> envvars.
>>>>>>>
>>>>>>>       - NativeMemoryTracking
>>>>>>>
>>>>>>>       - PrintFlagsWithComments
>>>>>>>>
>>>>>>>>> This is because these flags have to be processed before processing
>>>>>>>>> other
>>>>>>>>> flags, and no one ever bothered to do that for the flags coming
>>>>>>>>> from
>>>>>>>>> environment variables.  This patch fixes that problem.
>>>>>>>>>
>>>>>>>>> I have a test, but it is a modification to a JDK test instead of a
>>>>>>>>> HS
>>>>>>>>> test,
>>>>>>>>> so it can go in separately (I guess).
>>>>>>>>>
>>>>>>>>>   They can be pushed together to different repos in the same
>>>>>>>>> forest.
>>>>>>>>>
>>>>>>>>   Okay.  Here's the test change:
>>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/
>>>>>>>
>>>>>>>
>>>>>>>   As I said I'll have to get back to this. And hopefully someone
>>>>>>> else in
>>>>>>>
>>>>>>>> runtime will take a good look as well ;-)
>>>>>>>>
>>>>>>>>   I'd be happy to toss it over to whomever owns this, if anyone.
>>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Jeremy
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>     Bug:
>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8079301
>>>>>>>>>
>>>>>>>>> Webrev:
>>>>>>>>> http://cr.openjdk.java.net/~jmanson/8079301/webrev.00/
>>>>>>>>>
>>>>>>>>> Jeremy
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>

More information about the hotspot-runtime-dev mailing list