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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Jun 5 16:06:41 UTC 2013

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


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

More information about the hotspot-gc-dev mailing list