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

[Tao Mao]

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