URGENT: Request for review: 7122222: GC log is limited to 2G for 32-bit
Mikael Gerdin
mikael.gerdin at oracle.com
Thu Jun 6 09:10:24 UTC 2013
Dan,
On 2013-06-05 18:06, Daniel D. Daugherty wrote:
> 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.
I fail to see how:
#ifndef __USE_FILE_OFFSET64
/* Open a file and create a new stream for it.
This function is a possible cancellation point and therefore not
marked with __THROW. */
extern FILE *fopen (__const char *__restrict __filename,
__const char *__restrict __modes) __wur;
/* Open a file, replacing an existing stream with it.
This function is a possible cancellation point and therefore not
marked with __THROW. */
extern FILE *freopen (__const char *__restrict __filename,
__const char *__restrict __modes,
FILE *__restrict __stream) __wur;
#else
# ifdef __REDIRECT
extern FILE *__REDIRECT (fopen, (__const char *__restrict __filename,
__const char *__restrict __modes), fopen64)
__wur;
extern FILE *__REDIRECT (freopen, (__const char *__restrict __filename,
__const char *__restrict __modes, FILE *__restrict __stream), freopen64)
__wur;
# else
# define fopen fopen64
# define freopen freopen64
# endif
#endif
is not (at least in some cases) #define-based.
It is my understanding that if _FILE_OFFSET_BITS=64 then stdio.h will
"#define fopen fopen64"
* I think that it's confusing if we only do that for ostream.cpp
* If we ever add a "fopen()" call to a .hpp file which is included from
ostream.ccp then the caller of fopen will have different behavior in
based on which file it's called from.
I've written a small example to detail what kind of problems we could
get: https://gist.github.com/anonymous/b690f753401504f1f096
If ostream.cpp is the first to call into Logger::log we will get one
behavior, if another file does we get the default behavior.
Hope this makes my reasons for arguing about this a bit clearer.
/Mikael
>
> 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.
>>>>>>>>>>>>
>>>>>>>
>>>>>
>
More information about the hotspot-gc-dev
mailing list