URGENT: Request for review: 7122222: GC log is limited to 2G for 32-bit

[Daniel D. Daugherty]

On 6/5/13 10:02 AM, Mikael Gerdin wrote:
> Tao,
>
> On 2013-06-05 17:48, Tao Mao wrote:
>> Thank you for comments. One thing I want to point out is that the
>> current change has not touched on Windows code.
>>
>> Please see inline.
>>
>> Tao
>>
>> On 6/5/13 1:19 AM, Mikael Gerdin wrote:
>>>
>>>
>>> On 2013-06-05 02:21, Daniel D. Daugherty wrote:
>>>> OK, based on the largefiles.pdf write-up, your use of
>>>> _FILE_OFFSET_BITS=64
>>>> is going to cause ostream.o to bind to various 64-bit versions of some
>>>> functions. Based on my very hazy memory of my days in file system 
>>>> code,
>>>> this binding is going to affect ostream.o only unless ostream.o is
>>>> writing to some file with the foo64() function and some other code is
>>>> also writing to the same file at the same time with foo(). However,
>>>> if we
>>>> have two different functions writing to the same open file at the same
>>>> time, then we have bigger issues. :-)
>>>>
>>>> I'm good with these changes now. I agree that solving the problem of
>>>> setting _FILE_OFFSET_BITS=64 for the entire VM build doesn't have to
>>>> solved right now.
>>>
>>> I think this change is really scary, setting _FILE_OFFSET_BITS=64 when
>>> compiling ostream.cpp will effectively cause the headers to swap out
>>> the implementations of the f[open,tell,seek] to 64 bit versions for
>>> all headers that are included and inlined in ostream.cpp.
>>> Other parts of the code using the same headers will see different
>>> versions of the functions with different parameters due to off_t
>>> changing sizes.
>> The change is currently effective for Linux and Solaris if you look at
>> the file directories. Nothing changed for Windows and BSD, as they don't
>> need such change.
>
> Right.
> But if you use my suggested approach you would need to change calls to 
> fopen() in ostream.cpp to fopen_pd where
>
> if (linux || solaris) && 32bit
>     #define fopen_pd fopen64
> else
>     #define fopen_pd fopen
>
>>>
>>> I think that what we should do is to use the "Transitional compilation
>>> environment" detailed in §2.6.2 in largefiles.pdf and change the calls
>>> in ostream.cpp to use the appropriate f[open,tell,seek]64 functions
>>> directly. I feel this is especially important at this late stage in
>>> the release to make an explicit change instead of setting a #define
>>> which has propagating side-effects.
>> How do you see "propagating side-effects" and to where?
>
> _FILE_OFFSET_BITS=64 changes the definition of fopen for every file 
> including stdio.h.
>
> It's confusing when a call to "fopen()" in one file calls the 64 bit 
> version and in other files it doesn't.
>
> How will this work with precompiled headers? Which version of fopen 
> will be in the precompiled header file?

You're thinking this is like a "#define" and it is not.
The ostream.o binary will bind to the foo64() version of
functions and not the foo() version of functions. This
is not the same as using a #define.

Dan


>
>>>
>>> As Tao mentioned this will require us to handle the fact that there is
>>> no fopen64() call on Windows, that the CRT fopen() already seems to
>>> handle large files and that ftell64() and fseek64() have slightly
>>> different names on Windows. I don't think this is a large hurdle and I
>>> think we know how to solve this problem.
>> As I said, nothing was changed for Windows code.
>
> No, but to be consistent you'd need to update the ftell* fseek* to use 
> 64 bit versions, right?
>
> /Mikael
>
>>>
>>> /Mikael
>>>
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 6/4/13 6:06 PM, Tao Mao wrote:
>>>>> Thank you for review, Dan.
>>>>>
>>>>> I'll try to answer as much as I can. Please see inline.
>>>>>
>>>>> Thanks.
>>>>> Tao
>>>>>
>>>>> On 6/4/13 4:35 PM, Daniel D. Daugherty wrote:
>>>>>> > http://cr.openjdk.java.net/~tamao/7122222/webrev.00/
>>>>>>
>>>>>> Tao,
>>>>>>
>>>>>> I think the lack of response to this review request is the 
>>>>>> absolutely
>>>>>> strange nature of these changes. And I thought I put out some weird
>>>>>> code reviews... :-)
>>>>>>
>>>>>> make/linux/makefiles/vm.make
>>>>>>     Build ostream.o with _FILE_OFFSET_BITS==64 on Linux. Nothing
>>>>>>     obvious in this webrev about what this will mean so I took a
>>>>>>     look at src/share/vm/utilities/ostream.{c,h}pp and I see no
>>>>>>     use of _FILE_OFFSET_BITS in either of those source files.
>>>>>>
>>>>>>     Must be in the source files somewhere, but I can't find any
>>>>>>     use of _FILE_OFFSET_BITS in the entire hotspot source base.
>>>>>>
>>>>>> make/solaris/makefiles/vm.make
>>>>>> Build ostream.o with _FILE_OFFSET_BITS==64 on Solaris.
>>>>>>
>>>>>>     OK, I looked for _FILE_OFFSET_BITS in /usr/include on my
>>>>>>     Solaris box. Lots of references, but nothing that helps me
>>>>>>     understand what you're doing here.
>>>>>> src/os/solaris/vm/os_solaris.inline.hpp
>>>>>>     The addition of _FILE_OFFSET_BITS==64 means that the
>>>>>>     os::readdir() function will use the safer, multi-threaded
>>>>>>     version of readdir_r(). Seems fine to me.
>>>>>>
>>>>>> Here's what I need to know:
>>>>>>
>>>>>> - what effect does _FILE_OFFSET_BITS have on building 
>>>>>> ostream.{c,h}pp?
>>>>> _FILE_OFFSET_BITS is set to be picked by c++ compiler.
>>>>>
>>>>> For why we need to set _FILE_OFFSET_BITS==64 in this case, please
>>>>> refer to the following document
>>>>>
>>>>> This Sun White Paper
>>>>> (http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf)
>>>>> summarizes the usage of the flags on solaris (page "5-26"). And, it
>>>>> should apply to Linux the same way as was agreed across platforms
>>>>> (http://linuxmafia.com/faq/VALinux-kb/2gb-filesize-limit.html).
>>>>>
>>>>>> - if ostream.o has one idea about the value of _FILE_OFFSET_BITS
>>>>>>   what happens if another part of the VM has a different idea about
>>>>>>   the value of _FILE_OFFSET_BITS?
>>>>> _FILE_OFFSET_BITS is not set for other particular effects, but for
>>>>> extending the ability to deal with large files in ostream.{c,h}pp. 
>>>>> So,
>>>>> if other files have a different idea about _FILE_OFFSET_BITS, they
>>>>> can't deal with large files. No more no less.
>>>>>
>>>>>>
>>>>>> I saw this in the post to the Runtime alias:
>>>>>>
>>>>>> > Included runtime dev to see whether they have some idea to handle
>>>>>> > the compilation choices.
>>>>>>
>>>>>> And I still have no idea what you're asking here? What compilation
>>>>>> choices? Are you asking about your Makefile changes? Are asking
>>>>>> about defining _FILE_OFFSET_BITS for the entire build instead of
>>>>>> just one object (ostream.o)? Are you worried that this VM is going
>>>>>> to have mis-matched pieces and be unstable?
>>>>> "Are asking about defining _FILE_OFFSET_BITS for the entire build
>>>>> instead of just one object (ostream.o)?" is my main question I
>>>>> originally tried to ask.
>>>>>
>>>>>>
>>>>>> So I reviewed it, but I definitely can't approve it without more
>>>>>> info. I realize that you're up against the RDP2 limit, but this
>>>>>> change has too many open questions (for now)...
>>>>>>
>>>>>> BTW, it is not at all clear whether Win32 will be able to write a 
>>>>>> 2GB+
>>>>>> GC log or not. The conversation below didn't help me at all.
>>>>> I used a jdk7 (just any) to successfully generate a log file larger
>>>>> than 4GB. So, it shouldn't be a problem for Win32.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/4/13 5:03 PM, Tao Mao wrote:
>>>>>>> Since the changeset touched makefiles, I've included
>>>>>>> build-dev at openjdk.java.net .
>>>>>>>
>>>>>>> I need to push the hsx24 bug asap. Please review it.
>>>>>>>
>>>>>>> Thanks.
>>>>>>> Tao
>>>>>>>
>>>>>>> On 6/4/13 2:37 PM, Tao Mao wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> Need reviews to catch RDP2.
>>>>>>>>
>>>>>>>> The current webrev is a working solution to all platforms, Linux,
>>>>>>>> Windows, and Solaris.
>>>>>>>> http://cr.openjdk.java.net/~tamao/7122222/webrev.00/
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Tao
>>>>>>>>
>>>>>>>> On 5/30/13 10:21 AM, Tao Mao wrote:
>>>>>>>>> Included runtime dev to see whether they have some idea to handle
>>>>>>>>> the compilation choices.
>>>>>>>>>
>>>>>>>>> For now, it's been verified that the fix is functionally
>>>>>>>>> sufficient.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>> Tao
>>>>>>>>>
>>>>>>>>> On 5/29/13 5:27 PM, Tao Mao wrote:
>>>>>>>>>> Thank you, Mikael.
>>>>>>>>>>
>>>>>>>>>> Please see inline.
>>>>>>>>>>
>>>>>>>>>> Reviewers, please review it based on the following new
>>>>>>>>>> observation.
>>>>>>>>>>
>>>>>>>>>> Tao
>>>>>>>>>>
>>>>>>>>>> On 5/27/13 2:05 AM, Mikael Gerdin wrote:
>>>>>>>>>>> Tao,
>>>>>>>>>>>
>>>>>>>>>>> On 2013-05-25 02:19, Tao Mao wrote:
>>>>>>>>>>>> 7ux bug
>>>>>>>>>>>>
>>>>>>>>>>>> webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~tamao/7122222/webrev.00/
>>>>>>>>>>>>
>>>>>>>>>>>> changeset:
>>>>>>>>>>>> (1) make -D_FILE_OFFSET_BITS=64 only available to generating
>>>>>>>>>>>> ostream.o
>>>>>>>>>>>>
>>>>>>>>>>>> Why conservative rather than making -D_FILE_OFFSET_BITS=64
>>>>>>>>>>>> globally
>>>>>>>>>>>> applicable?
>>>>>>>>>>>>
>>>>>>>>>>>> Global setting of -D_FILE_OFFSET_BITS=64 on linux works fine;
>>>>>>>>>>>> however,
>>>>>>>>>>>> there are at least five code conflicts if introducing the flag
>>>>>>>>>>>> globally
>>>>>>>>>>>> to Solaris.
>>>>>>>>>>>>
>>>>>>>>>>>> One was resolved as in os_solaris.inline.hpp, but the rest 
>>>>>>>>>>>> four
>>>>>>>>>>>> files
>>>>>>>>>>>> had conflicts deep in c library. Even if they are excluded 
>>>>>>>>>>>> from
>>>>>>>>>>>> setting
>>>>>>>>>>>> -D_FILE_OFFSET_BITS=64, the compiled VM is corrupted.
>>>>>>>>>>>>
>>>>>>>>>>>> (2) For now, no Windows solution.
>>>>>>>>>>>> I haven't found any clean solution for solving this problem on
>>>>>>>>>>>> Windows.
>>>>>>>>>>>
>>>>>>>>>>> This seems like an insufficient fix if you can't make it 
>>>>>>>>>>> work on
>>>>>>>>>>> all platforms. I tried building with "-D_LARGEFILE64_SOURCE
>>>>>>>>>>> -D_FILE_OFFSET_BITS=64" ons Solaris and hit an #error in
>>>>>>>>>>> libelf.h saying it wasn't supported so I understand your 
>>>>>>>>>>> problem
>>>>>>>>>>> there.
>>>>>>>>>> Yes, that's my grief :( you touched them, a bunch of them. 
>>>>>>>>>> That's
>>>>>>>>>> why I chose to apply the flag only to the files (ostream.cpp and
>>>>>>>>>> ostream.hpp) I want the effect.
>>>>>>>>>>>
>>>>>>>>>>> Instead I suggest that you use the compatibility API described
>>>>>>>>>>> in lf64(5) on Solaris. This API consists of fopen64, ftell64 
>>>>>>>>>>> and
>>>>>>>>>>> friends and is exposed when "-D_LARGEFILE64_SOURCE" is set.
>>>>>>>>>>>
>>>>>>>>>>> The same "-D_LARGEFILE64_SOURCE" is available on Linux and has
>>>>>>>>>>> the added advantage of not changing any existing symbols and
>>>>>>>>>>> therefore we can set the define for all files instead of just
>>>>>>>>>>> ostream
>>>>>>>>>>>
>>>>>>>>>>> This approach has the added advantage that it more closely
>>>>>>>>>>> resembles the changes which will be needed for Windows anyway.
>>>>>>>>>>> Those changes would consist of changing calls to ftell/fseek to
>>>>>>>>>>> 64-bit versions and changing fopen to fopen64 on Solaris/Linux.
>>>>>>>>>> Both ways have pros and cons. The current implementation 
>>>>>>>>>> excludes
>>>>>>>>>> the usage of fopen64, providing portability (since there's no
>>>>>>>>>> fopen64 for Windows). Meanwhile, I understand your suggestion
>>>>>>>>>> provides other benefits.
>>>>>>>>>>
>>>>>>>>>> This Sun White Paper
>>>>>>>>>> (http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf) 
>>>>>>>>>>
>>>>>>>>>> summarizes
>>>>>>>>>> the usage of the flags on solaris (Page 5-26). And, it should
>>>>>>>>>> apply to Linux the same way as was agreed across platforms.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Since there is no fopen64 on Windows it seems that the default
>>>>>>>>>>> fopen already supports large files.
>>>>>>>>>> I tested, and you are correct that the 32-bit VM on Windows can
>>>>>>>>>> write beyond 2GB (and beyond 4GB). Thank you, it's solved "half
>>>>>>>>>> of my problem" :)
>>>>>>>>>>>
>>>>>>>>>>> /Mikael
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> test:
>>>>>>>>>>>> (1) Ability to write over 2g file for 32-bit builds were
>>>>>>>>>>>> verified on the
>>>>>>>>>>>> following configurations.
>>>>>>>>>>>>
>>>>>>>>>>>> Linux * i586
>>>>>>>>>>>> Solaris * i586
>>>>>>>>>>>> Solaris * sparc
>>>>>>>>>>>>
>>>>>>>>>>>> (2) Need a JPRT test for sanity check.
>>>>>>>>>>>
>>>>>>
>>>>