URGENT: Request for review: 7122222: GC log is limited to 2G for 32-bit
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jun 6 13:43:03 UTC 2013
On 6/6/13 3:10 AM, Mikael Gerdin wrote:
> 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.
Interesting. I'm guessing that the above is from stdio.h
that is not Solaris. Here's the relevant part of the Solaris
version:
#if defined(__PRAGMA_REDEFINE_EXTNAME)
/* large file compilation environment setup */
#if !defined(_LP64) && _FILE_OFFSET_BITS == 64
#pragma redefine_extname fopen fopen64
#pragma redefine_extname freopen freopen64
#pragma redefine_extname tmpfile tmpfile64
#pragma redefine_extname fgetpos fgetpos64
#pragma redefine_extname fsetpos fsetpos64
#pragma redefine_extname fseeko fseeko64
#pragma redefine_extname ftello ftello64
#endif /* !defined(_LP64) && _FILE_OFFSET_BITS == 64 */
/* In the LP64 compilation environment, all APIs are already large file */
#if defined(_LP64) && defined(_LARGEFILE64_SOURCE)
#pragma redefine_extname fopen64 fopen
#pragma redefine_extname freopen64 freopen
#pragma redefine_extname tmpfile64 tmpfile
#pragma redefine_extname fgetpos64 fgetpos
#pragma redefine_extname fsetpos64 fsetpos
#pragma redefine_extname fseeko64 fseeko
#pragma redefine_extname ftello64 ftello
#endif /* defined(_LP64) && defined(_LARGEFILE64_SOURCE) */
#endif /* __PRAGMA_REDEFINE_EXTNAME */
So I'm not worried about this change for Solaris (that much anyway).
I would say for the example that you quoted above, you need to
know about the "_REDIRECT" symbol and how that plays into the
compile system on whatever environment you got the stdio.h
quote from.
> It is my understanding that if _FILE_OFFSET_BITS=64 then stdio.h will
> "#define fopen fopen64"
Maybe. Maybe not. What you've quoted is not (yet) definitive.
> * I think that it's confusing if we only do that for ostream.cpp
I think we've both made the point that doing this just for
ostream.o can result in confusion.
> * 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'm not finding any reference to 'fopen' in any .hpp file in the
HotSpot code base, but, of course, that's not really your point.
Yes, we can screw things up in the future. I think that's a given.
> I've written a small example to detail what kind of problems we could
> get: https://gist.github.com/anonymous/b690f753401504f1f096
Yup. I "git" the example. I just don't quite understand why you
couldn't put the example here in the e-mail thread and had to drop
it on github...
> 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.
Yup, I understand your reasons and I think I alluded to other scenarios
earlier in the e-mail thread. There is no doubt that this is a risk
and that there could be unforeseen side effects.
Dan
> /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