RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
David Holmes
david.holmes at oracle.com
Fri Jul 10 06:53:23 UTC 2015
Hi Jeremy,
On 10/07/2015 10:23 AM, Jeremy Manson wrote:
> 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
I'm not sure you've changed anything in this regard but it seems to me
to me that if -XX:Flags=flags-file is specified on the command-line, and
the environment variable(s) then the last setting wins - which would be
the _JAVA_OPTIONS setting.
Or maybe that is the way this whole thing works ie last setting wins?
David
> 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